[Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements

[Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements

[BALATON Zoltan]

Some more improvements to sam460ex emulation:
- Implemented RTC (which also needed fixing I2C to work)
- Fix a problem in sm501 which led to AmigaOS slow down

v2:
- split patch 1 (i2c rewrite) into smaller pieces
- Added another sm501 patch

BALATON Zoltan (8):
  ppc4xx_i2c: Clean up and improve error logging
  ppc4xx_i2c: Move register state to private struct and remove    
    unimplemented sdata and intr registers
  ppc4xx_i2c: Implement directcntl register
  ppc4xx_i2c: Rewrite to model hardware more closely
  hw/timer: Add basic M41T80 emulation
  sam460ex: Add RTC device
  sm501: Do not clear read only bits when writing register
  sm501: Implement i2c part for reading monitor EDID

 MAINTAINERS                        |   1 +
 default-configs/ppc-softmmu.mak    |   3 +
 default-configs/ppcemb-softmmu.mak |   2 +
 default-configs/sh4-softmmu.mak    |   1 +
 default-configs/sh4eb-softmmu.mak  |   1 +
 hw/display/sm501.c                 | 142 ++++++++++++++-
 hw/i2c/ppc4xx_i2c.c                | 364 ++++++++++++++++++++-----------------
 hw/ppc/sam460ex.c                  |   1 +
 hw/timer/Makefile.objs             |   1 +
 hw/timer/m41t80.c                  | 117 ++++++++++++
 include/hw/i2c/ppc4xx_i2c.h        |  19 +-
 11 files changed, 457 insertions(+), 195 deletions(-)
 create mode 100644 hw/timer/m41t80.c

-- 
2.7.6

[Qemu-devel] [PATCH v2 6/8] sam460ex: Add RTC device

[BALATON Zoltan]

The Sam460ex has an M41T80 serial RTC chip on I2C bus 0 at address 0x68.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/sam460ex.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index bdc53d2..dc730cc 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -457,6 +457,7 @@ static void sam460ex_init(MachineState *machine)
     object_property_set_bool(OBJECT(dev), true, "realized", NULL);
     smbus_eeprom_init(i2c[0]->bus, 8, smbus_eeprom_buf, smbus_eeprom_size);
     g_free(smbus_eeprom_buf);
+    i2c_create_slave(i2c[0]->bus, "m41t80", 0x68);
 
     dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600800, uic[0][3]);
     i2c[1] = PPC4xx_I2C(dev);
-- 
2.7.6

[Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 4d7be45..7d0dc2f 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y
 
 # For Macs
 CONFIG_MAC=y
diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index 67d18b2..37af193 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index a68b5f7..5806209 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -30,6 +30,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
 
 #define PPC4xx_I2C_MEM_SIZE 18
 
@@ -46,7 +47,13 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 typedef struct {
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;
@@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->xtcntlss = value;
         break;
     case 16:
-        i2c->directcntl = value & 0x7;
+        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
+        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
+        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
         break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
@@ -343,6 +354,7 @@ static void ppc4xx_i2c_init(Object *o)
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
     sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
     s->bus = i2c_init_bus(DEVICE(s), "i2c");
+    r->bitbang = bitbang_i2c_init(s->bus);
 }
 
 static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
-- 
2.7.6

Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[David Gibson]

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

On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  default-configs/ppc-softmmu.mak    |  1 +
>  default-configs/ppcemb-softmmu.mak |  1 +
>  hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 4d7be45..7d0dc2f 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
>  CONFIG_SM501=y
>  CONFIG_IDE_SII3112=y
>  CONFIG_I2C=y
> +CONFIG_BITBANG_I2C=y
>  
>  # For Macs
>  CONFIG_MAC=y
> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> index 67d18b2..37af193 100644
> --- a/default-configs/ppcemb-softmmu.mak
> +++ b/default-configs/ppcemb-softmmu.mak
> @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
>  CONFIG_SM501=y
>  CONFIG_IDE_SII3112=y
>  CONFIG_I2C=y
> +CONFIG_BITBANG_I2C=y
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index a68b5f7..5806209 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -30,6 +30,7 @@
>  #include "cpu.h"
>  #include "hw/hw.h"
>  #include "hw/i2c/ppc4xx_i2c.h"
> +#include "bitbang_i2c.h"
>  
>  #define PPC4xx_I2C_MEM_SIZE 18
>  
> @@ -46,7 +47,13 @@
>  
>  #define IIC_XTCNTLSS_SRST   (1 << 0)
>  
> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
> +
>  typedef struct {
> +    bitbang_i2c_interface *bitbang;
>      uint8_t mdata;
>      uint8_t lmadr;
>      uint8_t hmadr;
> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>          i2c->xtcntlss = value;
>          break;
>      case 16:
> -        i2c->directcntl = value & 0x7;
> +        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> +        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> +        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?

> +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;

Last expression might be clearer as:
	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0

>          break;
>      default:
>          if (addr < PPC4xx_I2C_MEM_SIZE) {
> @@ -343,6 +354,7 @@ static void ppc4xx_i2c_init(Object *o)
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
>      sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
>      s->bus = i2c_init_bus(DEVICE(s), "i2c");
> +    r->bitbang = bitbang_i2c_init(s->bus);
>  }
>  
>  static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)

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

Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[BALATON Zoltan]

On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  default-configs/ppc-softmmu.mak    |  1 +
>>  default-configs/ppcemb-softmmu.mak |  1 +
>>  hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
>>  3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>> index 4d7be45..7d0dc2f 100644
>> --- a/default-configs/ppc-softmmu.mak
>> +++ b/default-configs/ppc-softmmu.mak
>> @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
>>  CONFIG_SM501=y
>>  CONFIG_IDE_SII3112=y
>>  CONFIG_I2C=y
>> +CONFIG_BITBANG_I2C=y
>>
>>  # For Macs
>>  CONFIG_MAC=y
>> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
>> index 67d18b2..37af193 100644
>> --- a/default-configs/ppcemb-softmmu.mak
>> +++ b/default-configs/ppcemb-softmmu.mak
>> @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
>>  CONFIG_SM501=y
>>  CONFIG_IDE_SII3112=y
>>  CONFIG_I2C=y
>> +CONFIG_BITBANG_I2C=y
>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>> index a68b5f7..5806209 100644
>> --- a/hw/i2c/ppc4xx_i2c.c
>> +++ b/hw/i2c/ppc4xx_i2c.c
>> @@ -30,6 +30,7 @@
>>  #include "cpu.h"
>>  #include "hw/hw.h"
>>  #include "hw/i2c/ppc4xx_i2c.h"
>> +#include "bitbang_i2c.h"
>>
>>  #define PPC4xx_I2C_MEM_SIZE 18
>>
>> @@ -46,7 +47,13 @@
>>
>>  #define IIC_XTCNTLSS_SRST   (1 << 0)
>>
>> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
>> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
>> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
>> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
>> +
>>  typedef struct {
>> +    bitbang_i2c_interface *bitbang;
>>      uint8_t mdata;
>>      uint8_t lmadr;
>>      uint8_t hmadr;
>> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>          i2c->xtcntlss = value;
>>          break;
>>      case 16:
>> -        i2c->directcntl = value & 0x7;
>> +        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
>> +        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>> +        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>
> Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
>
>> +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>> +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>
> Last expression might be clearer as:
> 	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0

I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit 
position in the register so I use that when accessing that bit but when I 
check for the values of a bit being 0 or 1 I don't use the define which is 
for something else, just happens to have value 1 as well.

If this does not explain your question and you think it's better to use 
defines here I can change that in next version, please let me know.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[David Gibson]

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

On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > ---
> > >  default-configs/ppc-softmmu.mak    |  1 +
> > >  default-configs/ppcemb-softmmu.mak |  1 +
> > >  hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
> > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> > > index 4d7be45..7d0dc2f 100644
> > > --- a/default-configs/ppc-softmmu.mak
> > > +++ b/default-configs/ppc-softmmu.mak
> > > @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
> > >  CONFIG_SM501=y
> > >  CONFIG_IDE_SII3112=y
> > >  CONFIG_I2C=y
> > > +CONFIG_BITBANG_I2C=y
> > > 
> > >  # For Macs
> > >  CONFIG_MAC=y
> > > diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> > > index 67d18b2..37af193 100644
> > > --- a/default-configs/ppcemb-softmmu.mak
> > > +++ b/default-configs/ppcemb-softmmu.mak
> > > @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
> > >  CONFIG_SM501=y
> > >  CONFIG_IDE_SII3112=y
> > >  CONFIG_I2C=y
> > > +CONFIG_BITBANG_I2C=y
> > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > index a68b5f7..5806209 100644
> > > --- a/hw/i2c/ppc4xx_i2c.c
> > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > @@ -30,6 +30,7 @@
> > >  #include "cpu.h"
> > >  #include "hw/hw.h"
> > >  #include "hw/i2c/ppc4xx_i2c.h"
> > > +#include "bitbang_i2c.h"
> > > 
> > >  #define PPC4xx_I2C_MEM_SIZE 18
> > > 
> > > @@ -46,7 +47,13 @@
> > > 
> > >  #define IIC_XTCNTLSS_SRST   (1 << 0)
> > > 
> > > +#define IIC_DIRECTCNTL_SDAC (1 << 3)
> > > +#define IIC_DIRECTCNTL_SCLC (1 << 2)
> > > +#define IIC_DIRECTCNTL_MSDA (1 << 1)
> > > +#define IIC_DIRECTCNTL_MSCL (1 << 0)
> > > +
> > >  typedef struct {
> > > +    bitbang_i2c_interface *bitbang;
> > >      uint8_t mdata;
> > >      uint8_t lmadr;
> > >      uint8_t hmadr;
> > > @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > >          i2c->xtcntlss = value;
> > >          break;
> > >      case 16:
> > > -        i2c->directcntl = value & 0x7;
> > > +        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> > > +        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> > > +        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
> > 
> > Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
> > 
> > > +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> > 
> > Last expression might be clearer as:
> > 	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
> 
> I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
> position in the register so I use that when accessing that bit but when I
> check for the values of a bit being 0 or 1 I don't use the define which is
> for something else, just happens to have value 1 as well.

Hmm.. but the bit is being store in i2c->directcntl, which means it
can be read back from the register in that position, no?

> If this does not explain your question and you think it's better to use
> defines here I can change that in next version, please let me know.
> 
> Regards,
> BALATON Zoltan
> 

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

Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[BALATON Zoltan]

On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
>> On Wed, 13 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  default-configs/ppc-softmmu.mak    |  1 +
>>>>  default-configs/ppcemb-softmmu.mak |  1 +
>>>>  hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
>>>>  3 files changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>>>> index 4d7be45..7d0dc2f 100644
>>>> --- a/default-configs/ppc-softmmu.mak
>>>> +++ b/default-configs/ppc-softmmu.mak
>>>> @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
>>>>  CONFIG_SM501=y
>>>>  CONFIG_IDE_SII3112=y
>>>>  CONFIG_I2C=y
>>>> +CONFIG_BITBANG_I2C=y
>>>>
>>>>  # For Macs
>>>>  CONFIG_MAC=y
>>>> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
>>>> index 67d18b2..37af193 100644
>>>> --- a/default-configs/ppcemb-softmmu.mak
>>>> +++ b/default-configs/ppcemb-softmmu.mak
>>>> @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
>>>>  CONFIG_SM501=y
>>>>  CONFIG_IDE_SII3112=y
>>>>  CONFIG_I2C=y
>>>> +CONFIG_BITBANG_I2C=y
>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>> index a68b5f7..5806209 100644
>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>> @@ -30,6 +30,7 @@
>>>>  #include "cpu.h"
>>>>  #include "hw/hw.h"
>>>>  #include "hw/i2c/ppc4xx_i2c.h"
>>>> +#include "bitbang_i2c.h"
>>>>
>>>>  #define PPC4xx_I2C_MEM_SIZE 18
>>>>
>>>> @@ -46,7 +47,13 @@
>>>>
>>>>  #define IIC_XTCNTLSS_SRST   (1 << 0)
>>>>
>>>> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
>>>> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
>>>> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
>>>> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
>>>> +
>>>>  typedef struct {
>>>> +    bitbang_i2c_interface *bitbang;
>>>>      uint8_t mdata;
>>>>      uint8_t lmadr;
>>>>      uint8_t hmadr;
>>>> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>>>          i2c->xtcntlss = value;
>>>>          break;
>>>>      case 16:
>>>> -        i2c->directcntl = value & 0x7;
>>>> +        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
>>>> +        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>>>> +        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>>>
>>> Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
>>>
>>>> +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>>>> +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>>>
>>> Last expression might be clearer as:
>>> 	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
>>
>> I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
>> position in the register so I use that when accessing that bit but when I
>> check for the values of a bit being 0 or 1 I don't use the define which is
>> for something else, just happens to have value 1 as well.
>
> Hmm.. but the bit is being store in i2c->directcntl, which means it
> can be read back from the register in that position, no?

Which of the above two do you mean?

In the first one I test for the 1/0 value set by the previous line before 
the bitbang_i2c_set call. This could be accessed as MSCL later but using 
that here would just make it longer and less obvious. If I want to be 
absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0) 
in this line too but that was just stored in the register one line before 
so I can reuse that here as well. Otherwise I could add another variable 
just for this bit value and use that in both lines but why make it more 
complicated for a simple 1 or 0 value?

In the second case using MSDA is really not correct because the level to 
set is defined by SDAC bit. The SDAC, SCLC bits are what the program sets 
to tell which states the two i2c lines should be and the MSDA, MSCL are 
read only bits that show what states the lines really are.

IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the 
directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c 
line.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[David Gibson]

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

On Wed, Jun 13, 2018 at 04:03:18PM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
> > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > ---
> > > > >  default-configs/ppc-softmmu.mak    |  1 +
> > > > >  default-configs/ppcemb-softmmu.mak |  1 +
> > > > >  hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
> > > > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> > > > > index 4d7be45..7d0dc2f 100644
> > > > > --- a/default-configs/ppc-softmmu.mak
> > > > > +++ b/default-configs/ppc-softmmu.mak
> > > > > @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
> > > > >  CONFIG_SM501=y
> > > > >  CONFIG_IDE_SII3112=y
> > > > >  CONFIG_I2C=y
> > > > > +CONFIG_BITBANG_I2C=y
> > > > > 
> > > > >  # For Macs
> > > > >  CONFIG_MAC=y
> > > > > diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> > > > > index 67d18b2..37af193 100644
> > > > > --- a/default-configs/ppcemb-softmmu.mak
> > > > > +++ b/default-configs/ppcemb-softmmu.mak
> > > > > @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
> > > > >  CONFIG_SM501=y
> > > > >  CONFIG_IDE_SII3112=y
> > > > >  CONFIG_I2C=y
> > > > > +CONFIG_BITBANG_I2C=y
> > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > > > index a68b5f7..5806209 100644
> > > > > --- a/hw/i2c/ppc4xx_i2c.c
> > > > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > > > @@ -30,6 +30,7 @@
> > > > >  #include "cpu.h"
> > > > >  #include "hw/hw.h"
> > > > >  #include "hw/i2c/ppc4xx_i2c.h"
> > > > > +#include "bitbang_i2c.h"
> > > > > 
> > > > >  #define PPC4xx_I2C_MEM_SIZE 18
> > > > > 
> > > > > @@ -46,7 +47,13 @@
> > > > > 
> > > > >  #define IIC_XTCNTLSS_SRST   (1 << 0)
> > > > > 
> > > > > +#define IIC_DIRECTCNTL_SDAC (1 << 3)
> > > > > +#define IIC_DIRECTCNTL_SCLC (1 << 2)
> > > > > +#define IIC_DIRECTCNTL_MSDA (1 << 1)
> > > > > +#define IIC_DIRECTCNTL_MSCL (1 << 0)
> > > > > +
> > > > >  typedef struct {
> > > > > +    bitbang_i2c_interface *bitbang;
> > > > >      uint8_t mdata;
> > > > >      uint8_t lmadr;
> > > > >      uint8_t hmadr;
> > > > > @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > > > >          i2c->xtcntlss = value;
> > > > >          break;
> > > > >      case 16:
> > > > > -        i2c->directcntl = value & 0x7;
> > > > > +        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> > > > > +        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> > > > > +        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
> > > > 
> > > > Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
> > > > 
> > > > > +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > > > +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> > > > 
> > > > Last expression might be clearer as:
> > > > 	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
> > > 
> > > I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
> > > position in the register so I use that when accessing that bit but when I
> > > check for the values of a bit being 0 or 1 I don't use the define which is
> > > for something else, just happens to have value 1 as well.
> > 
> > Hmm.. but the bit is being store in i2c->directcntl, which means it
> > can be read back from the register in that position, no?
> 
> Which of the above two do you mean?
> 
> In the first one I test for the 1/0 value set by the previous line before
> the bitbang_i2c_set call. This could be accessed as MSCL later but using
> that here would just make it longer and less obvious. If I want to be
> absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
> in this line too but that was just stored in the register one line before so
> I can reuse that here as well. Otherwise I could add another variable just
> for this bit value and use that in both lines but why make it more
> complicated for a simple 1 or 0 value?

Longer maybe, but I don't know about less obvious.  Actually I think
you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the
line setting i2c->directcntl, then the next line checking that bit to
pass it into bitbang_i2c_set.  The point is you're modifying the
effective register contents, so it makes sense to make it clearer
which bit of the register you're setting.

> In the second case using MSDA is really not correct because the level to set
> is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to
> tell which states the two i2c lines should be and the MSDA, MSCL are read
> only bits that show what states the lines really are.

Ok...

> IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
> directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
> line.

Uh.. what?  AFAICT, based on the result of bitbang_i2c_set() you're
updating the value of the MSDA (== 0x2) bit in i2c->directcntl
register state.  Why doesn't the symbolic name make sense here?

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

Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[BALATON Zoltan]

On Mon, 18 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 04:03:18PM +0200, BALATON Zoltan wrote:
>> On Wed, 13 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
>>>> On Wed, 13 Jun 2018, David Gibson wrote:
>>>>> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>>>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>>>> index a68b5f7..5806209 100644
>>>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>>>> @@ -30,6 +30,7 @@
>>>>>>  #include "cpu.h"
>>>>>>  #include "hw/hw.h"
>>>>>>  #include "hw/i2c/ppc4xx_i2c.h"
>>>>>> +#include "bitbang_i2c.h"
>>>>>>
>>>>>>  #define PPC4xx_I2C_MEM_SIZE 18
>>>>>>
>>>>>> @@ -46,7 +47,13 @@
>>>>>>
>>>>>>  #define IIC_XTCNTLSS_SRST   (1 << 0)
>>>>>>
>>>>>> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
>>>>>> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
>>>>>> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
>>>>>> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
>>>>>> +
>>>>>>  typedef struct {
>>>>>> +    bitbang_i2c_interface *bitbang;
>>>>>>      uint8_t mdata;
>>>>>>      uint8_t lmadr;
>>>>>>      uint8_t hmadr;
>>>>>> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>>>>>          i2c->xtcntlss = value;
>>>>>>          break;
>>>>>>      case 16:
>>>>>> -        i2c->directcntl = value & 0x7;
>>>>>> +        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
>>>>>> +        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>>>>>> +        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>>>>>
>>>>> Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
>>>>>
>>>>>> +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>>>>>> +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>>>>>
>>>>> Last expression might be clearer as:
>>>>> 	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
>>>>
>>>> I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
>>>> position in the register so I use that when accessing that bit but when I
>>>> check for the values of a bit being 0 or 1 I don't use the define which is
>>>> for something else, just happens to have value 1 as well.
>>>
>>> Hmm.. but the bit is being store in i2c->directcntl, which means it
>>> can be read back from the register in that position, no?
>>
>> Which of the above two do you mean?
>>
>> In the first one I test for the 1/0 value set by the previous line before
>> the bitbang_i2c_set call. This could be accessed as MSCL later but using
>> that here would just make it longer and less obvious. If I want to be
>> absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
>> in this line too but that was just stored in the register one line before so
>> I can reuse that here as well. Otherwise I could add another variable just
>> for this bit value and use that in both lines but why make it more
>> complicated for a simple 1 or 0 value?
>
> Longer maybe, but I don't know about less obvious.  Actually I think
> you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the
> line setting i2c->directcntl, then the next line checking that bit to
> pass it into bitbang_i2c_set.  The point is you're modifying the
> effective register contents, so it makes sense to make it clearer
> which bit of the register you're setting.

When setting the bit it's the value 1 so that's not the bit position, I 
think 1 : 0 is correct there. I've changed the next line in v4 I've just 
sent to the constant when checking the value of the MSCL bit.

>> In the second case using MSDA is really not correct because the level to set
>> is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to
>> tell which states the two i2c lines should be and the MSDA, MSCL are read
>> only bits that show what states the lines really are.
>
> Ok...
>
>> IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
>> directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
>> line.
>
> Uh.. what?  AFAICT, based on the result of bitbang_i2c_set() you're
> updating the value of the MSDA (== 0x2) bit in i2c->directcntl
> register state.  Why doesn't the symbolic name make sense here?

Sorry, I may not have been able to clearly say what I mean. I meant that 
IIC_DIRECTCNTL_MSDA means the bit in position 1 (numbering from LSB being 
bit number 0) which may have value 1 or 0. In cases I mean the value I use 
1 or 0. In case I refer to the bit position I use constants. In the line

bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

it should be the constant, just used 1 there for brevity because it's 
obvious from the previous line what's meant. I've changed this now. At 
other places the values of the bits are written as 1 or 0 so I think for 
those constants should not be needed.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[David Gibson]

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

On Tue, Jun 19, 2018 at 11:29:09AM +0200, BALATON Zoltan wrote:
> On Mon, 18 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 04:03:18PM +0200, BALATON Zoltan wrote:
> > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
> > > > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > > > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > > > > > index a68b5f7..5806209 100644
> > > > > > > --- a/hw/i2c/ppc4xx_i2c.c
> > > > > > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > > > > > @@ -30,6 +30,7 @@
> > > > > > >  #include "cpu.h"
> > > > > > >  #include "hw/hw.h"
> > > > > > >  #include "hw/i2c/ppc4xx_i2c.h"
> > > > > > > +#include "bitbang_i2c.h"
> > > > > > > 
> > > > > > >  #define PPC4xx_I2C_MEM_SIZE 18
> > > > > > > 
> > > > > > > @@ -46,7 +47,13 @@
> > > > > > > 
> > > > > > >  #define IIC_XTCNTLSS_SRST   (1 << 0)
> > > > > > > 
> > > > > > > +#define IIC_DIRECTCNTL_SDAC (1 << 3)
> > > > > > > +#define IIC_DIRECTCNTL_SCLC (1 << 2)
> > > > > > > +#define IIC_DIRECTCNTL_MSDA (1 << 1)
> > > > > > > +#define IIC_DIRECTCNTL_MSCL (1 << 0)
> > > > > > > +
> > > > > > >  typedef struct {
> > > > > > > +    bitbang_i2c_interface *bitbang;
> > > > > > >      uint8_t mdata;
> > > > > > >      uint8_t lmadr;
> > > > > > >      uint8_t hmadr;
> > > > > > > @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > > > > > >          i2c->xtcntlss = value;
> > > > > > >          break;
> > > > > > >      case 16:
> > > > > > > -        i2c->directcntl = value & 0x7;
> > > > > > > +        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> > > > > > > +        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> > > > > > > +        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
> > > > > > 
> > > > > > Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
> > > > > > 
> > > > > > > +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > > > > > +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> > > > > > 
> > > > > > Last expression might be clearer as:
> > > > > > 	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
> > > > > 
> > > > > I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
> > > > > position in the register so I use that when accessing that bit but when I
> > > > > check for the values of a bit being 0 or 1 I don't use the define which is
> > > > > for something else, just happens to have value 1 as well.
> > > > 
> > > > Hmm.. but the bit is being store in i2c->directcntl, which means it
> > > > can be read back from the register in that position, no?
> > > 
> > > Which of the above two do you mean?
> > > 
> > > In the first one I test for the 1/0 value set by the previous line before
> > > the bitbang_i2c_set call. This could be accessed as MSCL later but using
> > > that here would just make it longer and less obvious. If I want to be
> > > absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
> > > in this line too but that was just stored in the register one line before so
> > > I can reuse that here as well. Otherwise I could add another variable just
> > > for this bit value and use that in both lines but why make it more
> > > complicated for a simple 1 or 0 value?
> > 
> > Longer maybe, but I don't know about less obvious.  Actually I think
> > you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the
> > line setting i2c->directcntl, then the next line checking that bit to
> > pass it into bitbang_i2c_set.  The point is you're modifying the
> > effective register contents, so it makes sense to make it clearer
> > which bit of the register you're setting.
> 
> When setting the bit it's the value 1 so that's not the bit
> position,

Huh??  The constants aren't bit positions either, they're masks.  How
is IIC_DIRECTCNTL_MSCL wrong here?

> I
> think 1 : 0 is correct there.

Correct, sure, but less clear than it could be.

> I've changed the next line in v4 I've just
> sent to the constant when checking the value of the MSCL bit.
> 
> > > In the second case using MSDA is really not correct because the level to set
> > > is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to
> > > tell which states the two i2c lines should be and the MSDA, MSCL are read
> > > only bits that show what states the lines really are.
> > 
> > Ok...
> > 
> > > IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
> > > directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
> > > line.
> > 
> > Uh.. what?  AFAICT, based on the result of bitbang_i2c_set() you're
> > updating the value of the MSDA (== 0x2) bit in i2c->directcntl
> > register state.  Why doesn't the symbolic name make sense here?
> 
> Sorry, I may not have been able to clearly say what I mean. I meant that
> IIC_DIRECTCNTL_MSDA means the bit in position 1 (numbering from LSB being
> bit number 0) which may have value 1 or 0. In cases I mean the value I use 1
> or 0. In case I refer to the bit position I use constants. In the line
> 
> bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
> 
> it should be the constant, just used 1 there for brevity because it's
> obvious from the previous line what's meant.

Maybe, but using the constant is still clearer, and friendly to people
grepping the source.

> I've changed this now. At other
> places the values of the bits are written as 1 or 0 so I think for those
> constants should not be needed.

I have no idea what you mean by this.

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

Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[BALATON Zoltan]

On Wed, 20 Jun 2018, David Gibson wrote:
> On Tue, Jun 19, 2018 at 11:29:09AM +0200, BALATON Zoltan wrote:
>> On Mon, 18 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 13, 2018 at 04:03:18PM +0200, BALATON Zoltan wrote:
>>>> On Wed, 13 Jun 2018, David Gibson wrote:
>>>>> On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
>>>>>> On Wed, 13 Jun 2018, David Gibson wrote:
>>>>>>> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>>>>>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>>>>>> index a68b5f7..5806209 100644
>>>>>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>>>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>>>>>> @@ -30,6 +30,7 @@
>>>>>>>>  #include "cpu.h"
>>>>>>>>  #include "hw/hw.h"
>>>>>>>>  #include "hw/i2c/ppc4xx_i2c.h"
>>>>>>>> +#include "bitbang_i2c.h"
>>>>>>>>
>>>>>>>>  #define PPC4xx_I2C_MEM_SIZE 18
>>>>>>>>
>>>>>>>> @@ -46,7 +47,13 @@
>>>>>>>>
>>>>>>>>  #define IIC_XTCNTLSS_SRST   (1 << 0)
>>>>>>>>
>>>>>>>> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
>>>>>>>> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
>>>>>>>> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
>>>>>>>> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
>>>>>>>> +
>>>>>>>>  typedef struct {
>>>>>>>> +    bitbang_i2c_interface *bitbang;
>>>>>>>>      uint8_t mdata;
>>>>>>>>      uint8_t lmadr;
>>>>>>>>      uint8_t hmadr;
>>>>>>>> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>>>>>>>          i2c->xtcntlss = value;
>>>>>>>>          break;
>>>>>>>>      case 16:
>>>>>>>> -        i2c->directcntl = value & 0x7;
>>>>>>>> +        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
>>>>>>>> +        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>>>>>>>> +        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>>>>>>>
>>>>>>> Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
>>>>>>>
>>>>>>>> +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>>>>>>>> +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>>>>>>>
>>>>>>> Last expression might be clearer as:
>>>>>>> 	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
>>>>>>
>>>>>> I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
>>>>>> position in the register so I use that when accessing that bit but when I
>>>>>> check for the values of a bit being 0 or 1 I don't use the define which is
>>>>>> for something else, just happens to have value 1 as well.
>>>>>
>>>>> Hmm.. but the bit is being store in i2c->directcntl, which means it
>>>>> can be read back from the register in that position, no?
>>>>
>>>> Which of the above two do you mean?
>>>>
>>>> In the first one I test for the 1/0 value set by the previous line before
>>>> the bitbang_i2c_set call. This could be accessed as MSCL later but using
>>>> that here would just make it longer and less obvious. If I want to be
>>>> absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
>>>> in this line too but that was just stored in the register one line before so
>>>> I can reuse that here as well. Otherwise I could add another variable just
>>>> for this bit value and use that in both lines but why make it more
>>>> complicated for a simple 1 or 0 value?
>>>
>>> Longer maybe, but I don't know about less obvious.  Actually I think
>>> you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the
>>> line setting i2c->directcntl, then the next line checking that bit to
>>> pass it into bitbang_i2c_set.  The point is you're modifying the
>>> effective register contents, so it makes sense to make it clearer
>>> which bit of the register you're setting.
>>
>> When setting the bit it's the value 1 so that's not the bit
>> position,
>
> Huh??  The constants aren't bit positions either, they're masks.  How
> is IIC_DIRECTCNTL_MSCL wrong here?
>
>> I
>> think 1 : 0 is correct there.
>
> Correct, sure, but less clear than it could be.
>
>> I've changed the next line in v4 I've just
>> sent to the constant when checking the value of the MSCL bit.
>>
>>>> In the second case using MSDA is really not correct because the level to set
>>>> is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to
>>>> tell which states the two i2c lines should be and the MSDA, MSCL are read
>>>> only bits that show what states the lines really are.
>>>
>>> Ok...
>>>
>>>> IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
>>>> directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
>>>> line.
>>>
>>> Uh.. what?  AFAICT, based on the result of bitbang_i2c_set() you're
>>> updating the value of the MSDA (== 0x2) bit in i2c->directcntl
>>> register state.  Why doesn't the symbolic name make sense here?
>>
>> Sorry, I may not have been able to clearly say what I mean. I meant that
>> IIC_DIRECTCNTL_MSDA means the bit in position 1 (numbering from LSB being
>> bit number 0) which may have value 1 or 0. In cases I mean the value I use 1
>> or 0. In case I refer to the bit position I use constants. In the line
>>
>> bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>>
>> it should be the constant, just used 1 there for brevity because it's
>> obvious from the previous line what's meant.
>
> Maybe, but using the constant is still clearer, and friendly to people
> grepping the source.
>
>> I've changed this now. At other
>> places the values of the bits are written as 1 or 0 so I think for those
>> constants should not be needed.
>
> I have no idea what you mean by this.

OK, I'm lost now. Is v4 acceptable or are there any more changes you 
propose for that? Feel free to adjust it as you think would be more clear 
or tell me what to change if you want me to sumbit v5 of it.

Thank you,
BALATON Zoltan

[Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[BALATON Zoltan]

Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
of day is implemented. Setting time and RTC alarm are not supported.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 MAINTAINERS                     |   1 +
 default-configs/ppc-softmmu.mak |   1 +
 hw/timer/Makefile.objs          |   1 +
 hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 hw/timer/m41t80.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 41cd373..9e13bc1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
 L: qemu-ppc@nongnu.org
 S: Maintained
 F: hw/ide/sii3112.c
+F: hw/timer/m41t80.c
 
 SH4 Machines
 ------------
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 7d0dc2f..9fbaadc 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -27,6 +27,7 @@ CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
+CONFIG_M41T80=y
 
 # For Macs
 CONFIG_MAC=y
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 8b27a4b..e16b2b9 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
 common-obj-$(CONFIG_DS1338) += ds1338.o
 common-obj-$(CONFIG_HPET) += hpet.o
 common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
+common-obj-$(CONFIG_M41T80) += m41t80.o
 common-obj-$(CONFIG_M48T59) += m48t59.o
 ifeq ($(CONFIG_ISA_BUS),y)
 common-obj-$(CONFIG_M48T59) += m48t59-isa.o
diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
new file mode 100644
index 0000000..9dbdb1b
--- /dev/null
+++ b/hw/timer/m41t80.c
@@ -0,0 +1,117 @@
+/*
+ * M41T80 serial rtc emulation
+ *
+ * Copyright (c) 2018 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+#include "qemu/bcd.h"
+#include "hw/i2c/i2c.h"
+
+#define TYPE_M41T80 "m41t80"
+#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
+
+typedef struct M41t80State {
+    I2CSlave parent_obj;
+    int8_t addr;
+} M41t80State;
+
+static void m41t80_realize(DeviceState *dev, Error **errp)
+{
+    M41t80State *s = M41T80(dev);
+
+    s->addr = -1;
+}
+
+static int m41t80_send(I2CSlave *i2c, uint8_t data)
+{
+    M41t80State *s = M41T80(i2c);
+
+    if (s->addr < 0) {
+        s->addr = data;
+    } else {
+        s->addr++;
+    }
+    return 0;
+}
+
+static int m41t80_recv(I2CSlave *i2c)
+{
+    M41t80State *s = M41T80(i2c);
+    struct tm now;
+    qemu_timeval tv;
+
+    if (s->addr < 0) {
+        s->addr = 0;
+    }
+    if (s->addr >= 1 && s->addr <= 7) {
+        qemu_get_timedate(&now, -1);
+    }
+    switch (s->addr++) {
+    case 0:
+        qemu_gettimeofday(&tv);
+        return to_bcd(tv.tv_usec / 10000);
+    case 1:
+        return to_bcd(now.tm_sec);
+    case 2:
+        return to_bcd(now.tm_min);
+    case 3:
+        return to_bcd(now.tm_hour);
+    case 4:
+        return to_bcd(now.tm_wday);
+    case 5:
+        return to_bcd(now.tm_mday);
+    case 6:
+        return to_bcd(now.tm_mon + 1);
+    case 7:
+        return to_bcd(now.tm_year % 100);
+    case 8 ... 19:
+        qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
+                      __func__, s->addr - 1);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
+                      __func__, s->addr - 1);
+        return 0;
+    }
+}
+
+static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
+{
+    M41t80State *s = M41T80(i2c);
+
+    if (event == I2C_START_SEND) {
+        s->addr = -1;
+    }
+    return 0;
+}
+
+static void m41t80_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+
+    dc->realize = m41t80_realize;
+    sc->send = m41t80_send;
+    sc->recv = m41t80_recv;
+    sc->event = m41t80_event;
+}
+
+static const TypeInfo m41t80_info = {
+    .name          = TYPE_M41T80,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(M41t80State),
+    .class_init    = m41t80_class_init,
+};
+
+static void m41t80_register_types(void)
+{
+    type_register_static(&m41t80_info);
+}
+
+type_init(m41t80_register_types)
-- 
2.7.6

Re: [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[Philippe Mathieu-Daudé]

On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> of day is implemented. Setting time and RTC alarm are not supported.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  MAINTAINERS                     |   1 +
>  default-configs/ppc-softmmu.mak |   1 +
>  hw/timer/Makefile.objs          |   1 +
>  hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 120 insertions(+)
>  create mode 100644 hw/timer/m41t80.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 41cd373..9e13bc1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
>  L: qemu-ppc@nongnu.org
>  S: Maintained
>  F: hw/ide/sii3112.c
> +F: hw/timer/m41t80.c
>  
>  SH4 Machines
>  ------------
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 7d0dc2f..9fbaadc 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -27,6 +27,7 @@ CONFIG_SM501=y
>  CONFIG_IDE_SII3112=y
>  CONFIG_I2C=y
>  CONFIG_BITBANG_I2C=y
> +CONFIG_M41T80=y
>  
>  # For Macs
>  CONFIG_MAC=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 8b27a4b..e16b2b9 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
>  common-obj-$(CONFIG_DS1338) += ds1338.o
>  common-obj-$(CONFIG_HPET) += hpet.o
>  common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
> +common-obj-$(CONFIG_M41T80) += m41t80.o
>  common-obj-$(CONFIG_M48T59) += m48t59.o
>  ifeq ($(CONFIG_ISA_BUS),y)
>  common-obj-$(CONFIG_M48T59) += m48t59-isa.o
> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> new file mode 100644
> index 0000000..9dbdb1b
> --- /dev/null
> +++ b/hw/timer/m41t80.c
> @@ -0,0 +1,117 @@
> +/*
> + * M41T80 serial rtc emulation
> + *
> + * Copyright (c) 2018 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/timer.h"
> +#include "qemu/bcd.h"
> +#include "hw/i2c/i2c.h"
> +
> +#define TYPE_M41T80 "m41t80"
> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> +
> +typedef struct M41t80State {
> +    I2CSlave parent_obj;
> +    int8_t addr;
> +} M41t80State;
> +
> +static void m41t80_realize(DeviceState *dev, Error **errp)
> +{
> +    M41t80State *s = M41T80(dev);
> +
> +    s->addr = -1;
> +}
> +
> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> +{
> +    M41t80State *s = M41T80(i2c);
> +
> +    if (s->addr < 0) {
> +        s->addr = data;
> +    } else {
> +        s->addr++;
> +    }

What about adding enum i2c_event in M41t80State and use the enum here
rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
expected?

> +    return 0;
> +}
> +
> +static int m41t80_recv(I2CSlave *i2c)
> +{
> +    M41t80State *s = M41T80(i2c);
> +    struct tm now;
> +    qemu_timeval tv;

       int8_t addr;

> +
> +    if (s->addr < 0) {
> +        s->addr = 0;
> +    }

       addr = s->addr++;

So you can simply use 'addr' bellow.

> +    if (s->addr >= 1 && s->addr <= 7) {
> +        qemu_get_timedate(&now, -1);
> +    }
> +    switch (s->addr++) {
> +    case 0:
> +        qemu_gettimeofday(&tv);
> +        return to_bcd(tv.tv_usec / 10000);
> +    case 1:
> +        return to_bcd(now.tm_sec);
> +    case 2:
> +        return to_bcd(now.tm_min);
> +    case 3:
> +        return to_bcd(now.tm_hour);
> +    case 4:
> +        return to_bcd(now.tm_wday);
> +    case 5:
> +        return to_bcd(now.tm_mday);
> +    case 6:
> +        return to_bcd(now.tm_mon + 1);
> +    case 7:
> +        return to_bcd(now.tm_year % 100);
> +    case 8 ... 19:
> +        qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
> +                      __func__, s->addr - 1);
> +        return 0;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
> +                      __func__, s->addr - 1);
> +        return 0;
> +    }
> +}
> +
> +static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    M41t80State *s = M41T80(i2c);
> +
> +    if (event == I2C_START_SEND) {
> +        s->addr = -1;
> +    }
> +    return 0;
> +}
> +
> +static void m41t80_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> +
> +    dc->realize = m41t80_realize;
> +    sc->send = m41t80_send;
> +    sc->recv = m41t80_recv;
> +    sc->event = m41t80_event;
> +}
> +
> +static const TypeInfo m41t80_info = {
> +    .name          = TYPE_M41T80,
> +    .parent        = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(M41t80State),
> +    .class_init    = m41t80_class_init,
> +};
> +
> +static void m41t80_register_types(void)
> +{
> +    type_register_static(&m41t80_info);
> +}
> +
> +type_init(m41t80_register_types)
> 

Re: [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[BALATON Zoltan]

On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>> of day is implemented. Setting time and RTC alarm are not supported.
[...]
>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>> new file mode 100644
>> index 0000000..9dbdb1b
>> --- /dev/null
>> +++ b/hw/timer/m41t80.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * M41T80 serial rtc emulation
>> + *
>> + * Copyright (c) 2018 BALATON Zoltan
>> + *
>> + * This work is licensed under the GNU GPL license version 2 or later.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/timer.h"
>> +#include "qemu/bcd.h"
>> +#include "hw/i2c/i2c.h"
>> +
>> +#define TYPE_M41T80 "m41t80"
>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>> +
>> +typedef struct M41t80State {
>> +    I2CSlave parent_obj;
>> +    int8_t addr;
>> +} M41t80State;
>> +
>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>> +{
>> +    M41t80State *s = M41T80(dev);
>> +
>> +    s->addr = -1;
>> +}
>> +
>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>> +{
>> +    M41t80State *s = M41T80(i2c);
>> +
>> +    if (s->addr < 0) {
>> +        s->addr = data;
>> +    } else {
>> +        s->addr++;
>> +    }
>
> What about adding enum i2c_event in M41t80State and use the enum here
> rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> expected?

Thanks for the review. I guess we could add enum for device bytes and the 
special case -1 meaning no register address selected yet but this is a 
very simple device with only 20 bytes and the datasheet also lists them by 
number without naming them so I think we can also refer to them by number. 
Since the device has only this 20 bytes the case with 127 should also not 
be a problem as that's invalid address anyway. Or did you mean something 
else?

>> +    return 0;
>> +}
>> +
>> +static int m41t80_recv(I2CSlave *i2c)
>> +{
>> +    M41t80State *s = M41T80(i2c);
>> +    struct tm now;
>> +    qemu_timeval tv;
>
>       int8_t addr;
>
>> +
>> +    if (s->addr < 0) {
>> +        s->addr = 0;
>> +    }
>
>       addr = s->addr++;
>
> So you can simply use 'addr' bellow.

But that would mean numbers in switch cases below would be +1 compared to 
how they are listed in the register map in the datasheet but now they 
match so I'd leave it like this. (In case someone wants to check this with 
the datasheet, the one I could find on-line says the range of register 
number 4 (holding day of week) is 1-7 but U-Boot and clients expect 0-6 so 
I think it's a bug in the datasheet and clients were tested on hardware.)

Regards,
BALATON Zoltan

>> +    if (s->addr >= 1 && s->addr <= 7) {
>> +        qemu_get_timedate(&now, -1);
>> +    }
>> +    switch (s->addr++) {
>> +    case 0:
>> +        qemu_gettimeofday(&tv);
>> +        return to_bcd(tv.tv_usec / 10000);
>> +    case 1:
>> +        return to_bcd(now.tm_sec);
>> +    case 2:
>> +        return to_bcd(now.tm_min);
>> +    case 3:
>> +        return to_bcd(now.tm_hour);
>> +    case 4:
>> +        return to_bcd(now.tm_wday);
>> +    case 5:
>> +        return to_bcd(now.tm_mday);
>> +    case 6:
>> +        return to_bcd(now.tm_mon + 1);
>> +    case 7:
>> +        return to_bcd(now.tm_year % 100);
>> +    case 8 ... 19:
>> +        qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
>> +                      __func__, s->addr - 1);
>> +        return 0;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
>> +                      __func__, s->addr - 1);
>> +        return 0;
>> +    }
>> +}
>> +
>> +static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
>> +{
>> +    M41t80State *s = M41T80(i2c);
>> +
>> +    if (event == I2C_START_SEND) {
>> +        s->addr = -1;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static void m41t80_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
>> +
>> +    dc->realize = m41t80_realize;
>> +    sc->send = m41t80_send;
>> +    sc->recv = m41t80_recv;
>> +    sc->event = m41t80_event;
>> +}
>> +
>> +static const TypeInfo m41t80_info = {
>> +    .name          = TYPE_M41T80,
>> +    .parent        = TYPE_I2C_SLAVE,
>> +    .instance_size = sizeof(M41t80State),
>> +    .class_init    = m41t80_class_init,
>> +};
>> +
>> +static void m41t80_register_types(void)
>> +{
>> +    type_register_static(&m41t80_info);
>> +}
>> +
>> +type_init(m41t80_register_types)
>>
>
>

Re: [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[David Gibson]

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

On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> > On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > > Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> > > of day is implemented. Setting time and RTC alarm are not supported.
> [...]
> > > diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> > > new file mode 100644
> > > index 0000000..9dbdb1b
> > > --- /dev/null
> > > +++ b/hw/timer/m41t80.c
> > > @@ -0,0 +1,117 @@
> > > +/*
> > > + * M41T80 serial rtc emulation
> > > + *
> > > + * Copyright (c) 2018 BALATON Zoltan
> > > + *
> > > + * This work is licensed under the GNU GPL license version 2 or later.
> > > + *
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qemu/log.h"
> > > +#include "qemu/timer.h"
> > > +#include "qemu/bcd.h"
> > > +#include "hw/i2c/i2c.h"
> > > +
> > > +#define TYPE_M41T80 "m41t80"
> > > +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> > > +
> > > +typedef struct M41t80State {
> > > +    I2CSlave parent_obj;
> > > +    int8_t addr;
> > > +} M41t80State;
> > > +
> > > +static void m41t80_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    M41t80State *s = M41T80(dev);
> > > +
> > > +    s->addr = -1;
> > > +}
> > > +
> > > +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> > > +{
> > > +    M41t80State *s = M41T80(i2c);
> > > +
> > > +    if (s->addr < 0) {
> > > +        s->addr = data;
> > > +    } else {
> > > +        s->addr++;
> > > +    }
> > 
> > What about adding enum i2c_event in M41t80State and use the enum here
> > rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> > expected?
> 
> Thanks for the review. I guess we could add enum for device bytes and the
> special case -1 meaning no register address selected yet but this is a very
> simple device with only 20 bytes and the datasheet also lists them by number
> without naming them so I think we can also refer to them by number. Since
> the device has only this 20 bytes the case with 127 should also not be a
> problem as that's invalid address anyway. Or did you mean something else?

So, I'm not particularly in favour of adding extra state variables.

But is using addr < 0 safe here?  You're assigning the uint8_t data to
addr - could that result in a negative value?

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[BALATON Zoltan]

On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
>> On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
>>> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
>>>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>>>> of day is implemented. Setting time and RTC alarm are not supported.
>> [...]
>>>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>>>> new file mode 100644
>>>> index 0000000..9dbdb1b
>>>> --- /dev/null
>>>> +++ b/hw/timer/m41t80.c
>>>> @@ -0,0 +1,117 @@
>>>> +/*
>>>> + * M41T80 serial rtc emulation
>>>> + *
>>>> + * Copyright (c) 2018 BALATON Zoltan
>>>> + *
>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>> + *
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qemu/log.h"
>>>> +#include "qemu/timer.h"
>>>> +#include "qemu/bcd.h"
>>>> +#include "hw/i2c/i2c.h"
>>>> +
>>>> +#define TYPE_M41T80 "m41t80"
>>>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>>>> +
>>>> +typedef struct M41t80State {
>>>> +    I2CSlave parent_obj;
>>>> +    int8_t addr;
>>>> +} M41t80State;
>>>> +
>>>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    M41t80State *s = M41T80(dev);
>>>> +
>>>> +    s->addr = -1;
>>>> +}
>>>> +
>>>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>>>> +{
>>>> +    M41t80State *s = M41T80(i2c);
>>>> +
>>>> +    if (s->addr < 0) {
>>>> +        s->addr = data;
>>>> +    } else {
>>>> +        s->addr++;
>>>> +    }
>>>
>>> What about adding enum i2c_event in M41t80State and use the enum here
>>> rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
>>> expected?
>>
>> Thanks for the review. I guess we could add enum for device bytes and the
>> special case -1 meaning no register address selected yet but this is a very
>> simple device with only 20 bytes and the datasheet also lists them by number
>> without naming them so I think we can also refer to them by number. Since
>> the device has only this 20 bytes the case with 127 should also not be a
>> problem as that's invalid address anyway. Or did you mean something else?
>
> So, I'm not particularly in favour of adding extra state variables.
>
> But is using addr < 0 safe here?  You're assigning the uint8_t data to
> addr - could that result in a negative value?

Why wouldn't it be safe with the expected values for register address 
between 0-19? If the guest sends garbage values over 127 it will either 
result in invalid register or unselected register and lead to an error 
when trying to read/write that register so I don't see what other problem 
this may cause.

The addr < 0 is to check if no address was selected before (on creating 
the device and when sending first value from host addr is set to -1. In 
this case first write will set register address, then subsequent 
reads/writes increment register address as the datasheet says).

Regards,
BALATON Zoltan

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[David Gibson]

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

On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> > > On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> > > > On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > > > > Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> > > > > of day is implemented. Setting time and RTC alarm are not supported.
> > > [...]
> > > > > diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> > > > > new file mode 100644
> > > > > index 0000000..9dbdb1b
> > > > > --- /dev/null
> > > > > +++ b/hw/timer/m41t80.c
> > > > > @@ -0,0 +1,117 @@
> > > > > +/*
> > > > > + * M41T80 serial rtc emulation
> > > > > + *
> > > > > + * Copyright (c) 2018 BALATON Zoltan
> > > > > + *
> > > > > + * This work is licensed under the GNU GPL license version 2 or later.
> > > > > + *
> > > > > + */
> > > > > +
> > > > > +#include "qemu/osdep.h"
> > > > > +#include "qemu/log.h"
> > > > > +#include "qemu/timer.h"
> > > > > +#include "qemu/bcd.h"
> > > > > +#include "hw/i2c/i2c.h"
> > > > > +
> > > > > +#define TYPE_M41T80 "m41t80"
> > > > > +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> > > > > +
> > > > > +typedef struct M41t80State {
> > > > > +    I2CSlave parent_obj;
> > > > > +    int8_t addr;
> > > > > +} M41t80State;
> > > > > +
> > > > > +static void m41t80_realize(DeviceState *dev, Error **errp)
> > > > > +{
> > > > > +    M41t80State *s = M41T80(dev);
> > > > > +
> > > > > +    s->addr = -1;
> > > > > +}
> > > > > +
> > > > > +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> > > > > +{
> > > > > +    M41t80State *s = M41T80(i2c);
> > > > > +
> > > > > +    if (s->addr < 0) {
> > > > > +        s->addr = data;
> > > > > +    } else {
> > > > > +        s->addr++;
> > > > > +    }
> > > > 
> > > > What about adding enum i2c_event in M41t80State and use the enum here
> > > > rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> > > > expected?
> > > 
> > > Thanks for the review. I guess we could add enum for device bytes and the
> > > special case -1 meaning no register address selected yet but this is a very
> > > simple device with only 20 bytes and the datasheet also lists them by number
> > > without naming them so I think we can also refer to them by number. Since
> > > the device has only this 20 bytes the case with 127 should also not be a
> > > problem as that's invalid address anyway. Or did you mean something else?
> > 
> > So, I'm not particularly in favour of adding extra state variables.
> > 
> > But is using addr < 0 safe here?  You're assigning the uint8_t data to
> > addr - could that result in a negative value?
> 
> Why wouldn't it be safe with the expected values for register address
> between 0-19? If the guest sends garbage values over 127 it will either
> result in invalid register or unselected register and lead to an error when
> trying to read/write that register so I don't see what other problem this
> may cause.

Ok, but where is that enforced?

> The addr < 0 is to check if no address was selected before (on creating the
> device and when sending first value from host addr is set to -1. In this
> case first write will set register address, then subsequent reads/writes
> increment register address as the datasheet says).
> 
> Regards,
> BALATON Zoltan


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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[BALATON Zoltan]

On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
>> On Wed, 13 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
>>>> On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
>>>>> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
>>>>>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>>>>>> of day is implemented. Setting time and RTC alarm are not supported.
>>>> [...]
>>>>>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>>>>>> new file mode 100644
>>>>>> index 0000000..9dbdb1b
>>>>>> --- /dev/null
>>>>>> +++ b/hw/timer/m41t80.c
>>>>>> @@ -0,0 +1,117 @@
>>>>>> +/*
>>>>>> + * M41T80 serial rtc emulation
>>>>>> + *
>>>>>> + * Copyright (c) 2018 BALATON Zoltan
>>>>>> + *
>>>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>>>> + *
>>>>>> + */
>>>>>> +
>>>>>> +#include "qemu/osdep.h"
>>>>>> +#include "qemu/log.h"
>>>>>> +#include "qemu/timer.h"
>>>>>> +#include "qemu/bcd.h"
>>>>>> +#include "hw/i2c/i2c.h"
>>>>>> +
>>>>>> +#define TYPE_M41T80 "m41t80"
>>>>>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>>>>>> +
>>>>>> +typedef struct M41t80State {
>>>>>> +    I2CSlave parent_obj;
>>>>>> +    int8_t addr;
>>>>>> +} M41t80State;
>>>>>> +
>>>>>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>>>>>> +{
>>>>>> +    M41t80State *s = M41T80(dev);
>>>>>> +
>>>>>> +    s->addr = -1;
>>>>>> +}
>>>>>> +
>>>>>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>>>>>> +{
>>>>>> +    M41t80State *s = M41T80(i2c);
>>>>>> +
>>>>>> +    if (s->addr < 0) {
>>>>>> +        s->addr = data;
>>>>>> +    } else {
>>>>>> +        s->addr++;
>>>>>> +    }
>>>>>
>>>>> What about adding enum i2c_event in M41t80State and use the enum here
>>>>> rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
>>>>> expected?
>>>>
>>>> Thanks for the review. I guess we could add enum for device bytes and the
>>>> special case -1 meaning no register address selected yet but this is a very
>>>> simple device with only 20 bytes and the datasheet also lists them by number
>>>> without naming them so I think we can also refer to them by number. Since
>>>> the device has only this 20 bytes the case with 127 should also not be a
>>>> problem as that's invalid address anyway. Or did you mean something else?
>>>
>>> So, I'm not particularly in favour of adding extra state variables.
>>>
>>> But is using addr < 0 safe here?  You're assigning the uint8_t data to
>>> addr - could that result in a negative value?
>>
>> Why wouldn't it be safe with the expected values for register address
>> between 0-19? If the guest sends garbage values over 127 it will either
>> result in invalid register or unselected register and lead to an error when
>> trying to read/write that register so I don't see what other problem this
>> may cause.
>
> Ok, but where is that enforced?

I don't see the problem. The addr register selects the register to read or 
write. It is set by the first write when the device is accessed the first 
time (this is denoted by addr == -1 (or really any negative value). The 
device has 20 registers and trying to read any register outside addr 
between 0-19 will result in returning 0 and logging a warning about 
invalid register in m41t80_recv. What could fail here when guest sends 
garbage? It will set addr to invalid value and try to read non-exitent 
register and get an error just like for any other nonexistent value of 
addr (or start to read from register 0 if it manages to set a negative 
value). All writes of registers are ignored currently (except setting addr 
by the first write). What should be enforced here?

Regards,
BALATON Zoltan

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[David Gibson]

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

On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
> > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> > > > > On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> > > > > > On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > > > > > > Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> > > > > > > of day is implemented. Setting time and RTC alarm are not supported.
> > > > > [...]
> > > > > > > diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> > > > > > > new file mode 100644
> > > > > > > index 0000000..9dbdb1b
> > > > > > > --- /dev/null
> > > > > > > +++ b/hw/timer/m41t80.c
> > > > > > > @@ -0,0 +1,117 @@
> > > > > > > +/*
> > > > > > > + * M41T80 serial rtc emulation
> > > > > > > + *
> > > > > > > + * Copyright (c) 2018 BALATON Zoltan
> > > > > > > + *
> > > > > > > + * This work is licensed under the GNU GPL license version 2 or later.
> > > > > > > + *
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include "qemu/osdep.h"
> > > > > > > +#include "qemu/log.h"
> > > > > > > +#include "qemu/timer.h"
> > > > > > > +#include "qemu/bcd.h"
> > > > > > > +#include "hw/i2c/i2c.h"
> > > > > > > +
> > > > > > > +#define TYPE_M41T80 "m41t80"
> > > > > > > +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> > > > > > > +
> > > > > > > +typedef struct M41t80State {
> > > > > > > +    I2CSlave parent_obj;
> > > > > > > +    int8_t addr;
> > > > > > > +} M41t80State;
> > > > > > > +
> > > > > > > +static void m41t80_realize(DeviceState *dev, Error **errp)
> > > > > > > +{
> > > > > > > +    M41t80State *s = M41T80(dev);
> > > > > > > +
> > > > > > > +    s->addr = -1;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> > > > > > > +{
> > > > > > > +    M41t80State *s = M41T80(i2c);
> > > > > > > +
> > > > > > > +    if (s->addr < 0) {
> > > > > > > +        s->addr = data;
> > > > > > > +    } else {
> > > > > > > +        s->addr++;
> > > > > > > +    }
> > > > > > 
> > > > > > What about adding enum i2c_event in M41t80State and use the enum here
> > > > > > rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> > > > > > expected?
> > > > > 
> > > > > Thanks for the review. I guess we could add enum for device bytes and the
> > > > > special case -1 meaning no register address selected yet but this is a very
> > > > > simple device with only 20 bytes and the datasheet also lists them by number
> > > > > without naming them so I think we can also refer to them by number. Since
> > > > > the device has only this 20 bytes the case with 127 should also not be a
> > > > > problem as that's invalid address anyway. Or did you mean something else?
> > > > 
> > > > So, I'm not particularly in favour of adding extra state variables.
> > > > 
> > > > But is using addr < 0 safe here?  You're assigning the uint8_t data to
> > > > addr - could that result in a negative value?
> > > 
> > > Why wouldn't it be safe with the expected values for register address
> > > between 0-19? If the guest sends garbage values over 127 it will either
> > > result in invalid register or unselected register and lead to an error when
> > > trying to read/write that register so I don't see what other problem this
> > > may cause.
> > 
> > Ok, but where is that enforced?
> 
> I don't see the problem. The addr register selects the register to read or
> write. It is set by the first write when the device is accessed the first
> time (this is denoted by addr == -1 (or really any negative value). The
> device has 20 registers and trying to read any register outside addr between
> 0-19 will result in returning 0 and logging a warning about invalid register
> in m41t80_recv. What could fail here when guest sends garbage? It will set
> addr to invalid value and try to read non-exitent register and get an error
> just like for any other nonexistent value of addr (or start to read from
> register 0 if it manages to set a negative value). All writes of registers
> are ignored currently (except setting addr by the first write). What should
> be enforced here?

Ah, I see your point.  I mean strictly we should match the hardware
behaviour if you write garbage addresses here, but really I don't
think it matters much.

Ok, it should be fine.

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[BALATON Zoltan]

On Thu, 14 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
>> I don't see the problem. The addr register selects the register to read or
>> write. It is set by the first write when the device is accessed the first
>> time (this is denoted by addr == -1 (or really any negative value). The
>> device has 20 registers and trying to read any register outside addr between
>> 0-19 will result in returning 0 and logging a warning about invalid register
>> in m41t80_recv. What could fail here when guest sends garbage? It will set
>> addr to invalid value and try to read non-exitent register and get an error
>> just like for any other nonexistent value of addr (or start to read from
>> register 0 if it manages to set a negative value). All writes of registers
>> are ignored currently (except setting addr by the first write). What should
>> be enforced here?
>
> Ah, I see your point.  I mean strictly we should match the hardware
> behaviour if you write garbage addresses here, but really I don't
> think it matters much.

Problem is like usual I have no idea what the real hardware does. I've 
only seen the datasheet, never seen real device and have no way to test. 
All the clients I've tested seem to be OK with the current emulation so 
unless someone has more info on how this should work I think we can live 
with this version and then fix it later if found to be needed.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[David Gibson]

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

On Thu, Jun 14, 2018 at 09:54:41AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
> > > I don't see the problem. The addr register selects the register to read or
> > > write. It is set by the first write when the device is accessed the first
> > > time (this is denoted by addr == -1 (or really any negative value). The
> > > device has 20 registers and trying to read any register outside addr between
> > > 0-19 will result in returning 0 and logging a warning about invalid register
> > > in m41t80_recv. What could fail here when guest sends garbage? It will set
> > > addr to invalid value and try to read non-exitent register and get an error
> > > just like for any other nonexistent value of addr (or start to read from
> > > register 0 if it manages to set a negative value). All writes of registers
> > > are ignored currently (except setting addr by the first write). What should
> > > be enforced here?
> > 
> > Ah, I see your point.  I mean strictly we should match the hardware
> > behaviour if you write garbage addresses here, but really I don't
> > think it matters much.
> 
> Problem is like usual I have no idea what the real hardware does. I've only
> seen the datasheet, never seen real device and have no way to test. All the
> clients I've tested seem to be OK with the current emulation so unless
> someone has more info on how this should work I think we can live with this
> version and then fix it later if found to be needed.

Ok, fair enough.

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[Cédric Le Goater]

On 06/06/2018 03:31 PM, BALATON Zoltan wrote:
> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> of day is implemented. Setting time and RTC alarm are not supported.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  MAINTAINERS                     |   1 +
>  default-configs/ppc-softmmu.mak |   1 +
>  hw/timer/Makefile.objs          |   1 +
>  hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 120 insertions(+)
>  create mode 100644 hw/timer/m41t80.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 41cd373..9e13bc1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
>  L: qemu-ppc@nongnu.org
>  S: Maintained
>  F: hw/ide/sii3112.c
> +F: hw/timer/m41t80.c
>  
>  SH4 Machines
>  ------------
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 7d0dc2f..9fbaadc 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -27,6 +27,7 @@ CONFIG_SM501=y
>  CONFIG_IDE_SII3112=y
>  CONFIG_I2C=y
>  CONFIG_BITBANG_I2C=y
> +CONFIG_M41T80=y
>  
>  # For Macs
>  CONFIG_MAC=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 8b27a4b..e16b2b9 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
>  common-obj-$(CONFIG_DS1338) += ds1338.o
>  common-obj-$(CONFIG_HPET) += hpet.o
>  common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
> +common-obj-$(CONFIG_M41T80) += m41t80.o
>  common-obj-$(CONFIG_M48T59) += m48t59.o
>  ifeq ($(CONFIG_ISA_BUS),y)
>  common-obj-$(CONFIG_M48T59) += m48t59-isa.o
> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> new file mode 100644
> index 0000000..9dbdb1b
> --- /dev/null
> +++ b/hw/timer/m41t80.c
> @@ -0,0 +1,117 @@
> +/*
> + * M41T80 serial rtc emulation
> + *
> + * Copyright (c) 2018 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/timer.h"
> +#include "qemu/bcd.h"
> +#include "hw/i2c/i2c.h"
> +
> +#define TYPE_M41T80 "m41t80"
> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> +
> +typedef struct M41t80State {
> +    I2CSlave parent_obj;
> +    int8_t addr;
> +} M41t80State;
> +
> +static void m41t80_realize(DeviceState *dev, Error **errp)
> +{
> +    M41t80State *s = M41T80(dev);
> +
> +    s->addr = -1;
> +}
> +
> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> +{
> +    M41t80State *s = M41T80(i2c);
> +
> +    if (s->addr < 0) {
> +        s->addr = data;
> +    } else {
> +        s->addr++;
> +    }
> +    return 0;
> +}
> +
> +static int m41t80_recv(I2CSlave *i2c)
> +{
> +    M41t80State *s = M41T80(i2c);
> +    struct tm now;
> +    qemu_timeval tv;
> +
> +    if (s->addr < 0) {
> +        s->addr = 0;
> +    }
> +    if (s->addr >= 1 && s->addr <= 7) {
> +        qemu_get_timedate(&now, -1);
> +    }
> +    switch (s->addr++) {

you could use some define to name the registers :

> +    case 0:
> +        qemu_gettimeofday(&tv);
> +        return to_bcd(tv.tv_usec / 10000);> +    case 1:
> +        return to_bcd(now.tm_sec);
> +    case 2:
> +        return to_bcd(now.tm_min);
> +    case 3:
> +        return to_bcd(now.tm_hour);

There is an interesting century bit in specs.

> +    case 4:
> +        return to_bcd(now.tm_wday);
> +    case 5:
> +        return to_bcd(now.tm_mday);
> +    case 6:
> +        return to_bcd(now.tm_mon + 1);
> +    case 7:
> +        return to_bcd(now.tm_year % 100);
> +    case 8 ... 19:
> +        qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",

is the beginning \n needed ?

Thanks,

C.
 
> +                      __func__, s->addr - 1);
> +        return 0;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
> +                      __func__, s->addr - 1);
> +        return 0;
> +    }
> +}
> +
> +static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    M41t80State *s = M41T80(i2c);
> +
> +    if (event == I2C_START_SEND) {
> +        s->addr = -1;
> +    }
> +    return 0;
> +}
> +
> +static void m41t80_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> +
> +    dc->realize = m41t80_realize;
> +    sc->send = m41t80_send;
> +    sc->recv = m41t80_recv;
> +    sc->event = m41t80_event;
> +}
> +
> +static const TypeInfo m41t80_info = {
> +    .name          = TYPE_M41T80,
> +    .parent        = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(M41t80State),
> +    .class_init    = m41t80_class_init,
> +};
> +
> +static void m41t80_register_types(void)
> +{
> +    type_register_static(&m41t80_info);
> +}
> +
> +type_init(m41t80_register_types)
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[BALATON Zoltan]

On Fri, 8 Jun 2018, Cédric Le Goater wrote:
> On 06/06/2018 03:31 PM, BALATON Zoltan wrote:
>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>> of day is implemented. Setting time and RTC alarm are not supported.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  MAINTAINERS                     |   1 +
>>  default-configs/ppc-softmmu.mak |   1 +
>>  hw/timer/Makefile.objs          |   1 +
>>  hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 120 insertions(+)
>>  create mode 100644 hw/timer/m41t80.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 41cd373..9e13bc1 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
>>  L: qemu-ppc@nongnu.org
>>  S: Maintained
>>  F: hw/ide/sii3112.c
>> +F: hw/timer/m41t80.c
>>
>>  SH4 Machines
>>  ------------
>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>> index 7d0dc2f..9fbaadc 100644
>> --- a/default-configs/ppc-softmmu.mak
>> +++ b/default-configs/ppc-softmmu.mak
>> @@ -27,6 +27,7 @@ CONFIG_SM501=y
>>  CONFIG_IDE_SII3112=y
>>  CONFIG_I2C=y
>>  CONFIG_BITBANG_I2C=y
>> +CONFIG_M41T80=y
>>
>>  # For Macs
>>  CONFIG_MAC=y
>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>> index 8b27a4b..e16b2b9 100644
>> --- a/hw/timer/Makefile.objs
>> +++ b/hw/timer/Makefile.objs
>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
>>  common-obj-$(CONFIG_DS1338) += ds1338.o
>>  common-obj-$(CONFIG_HPET) += hpet.o
>>  common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
>> +common-obj-$(CONFIG_M41T80) += m41t80.o
>>  common-obj-$(CONFIG_M48T59) += m48t59.o
>>  ifeq ($(CONFIG_ISA_BUS),y)
>>  common-obj-$(CONFIG_M48T59) += m48t59-isa.o
>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>> new file mode 100644
>> index 0000000..9dbdb1b
>> --- /dev/null
>> +++ b/hw/timer/m41t80.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * M41T80 serial rtc emulation
>> + *
>> + * Copyright (c) 2018 BALATON Zoltan
>> + *
>> + * This work is licensed under the GNU GPL license version 2 or later.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/timer.h"
>> +#include "qemu/bcd.h"
>> +#include "hw/i2c/i2c.h"
>> +
>> +#define TYPE_M41T80 "m41t80"
>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>> +
>> +typedef struct M41t80State {
>> +    I2CSlave parent_obj;
>> +    int8_t addr;
>> +} M41t80State;
>> +
>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>> +{
>> +    M41t80State *s = M41T80(dev);
>> +
>> +    s->addr = -1;
>> +}
>> +
>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>> +{
>> +    M41t80State *s = M41T80(i2c);
>> +
>> +    if (s->addr < 0) {
>> +        s->addr = data;
>> +    } else {
>> +        s->addr++;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int m41t80_recv(I2CSlave *i2c)
>> +{
>> +    M41t80State *s = M41T80(i2c);
>> +    struct tm now;
>> +    qemu_timeval tv;
>> +
>> +    if (s->addr < 0) {
>> +        s->addr = 0;
>> +    }
>> +    if (s->addr >= 1 && s->addr <= 7) {
>> +        qemu_get_timedate(&now, -1);
>> +    }
>> +    switch (s->addr++) {
>
> you could use some define to name the registers :

This was also suggested by Philippe Mathieu-Daudé and my answer to that 
was that I don't feel like I want to come up with names the datasheet does 
not have either. I think this device is simple enough with just 20 
consecutively numbered registers that appear only in these switch cases by 
number as in the datasheet table so that I don't want to make it more 
difficult to read by encrypting these numbers behind some arbitrary 
defines without a good reason. They are also so simple that it's clear 
from the usually one line implementation what they do so that's also not a 
good reason to name them.

>> +    case 0:
>> +        qemu_gettimeofday(&tv);
>> +        return to_bcd(tv.tv_usec / 10000);> +    case 1:
>> +        return to_bcd(now.tm_sec);
>> +    case 2:
>> +        return to_bcd(now.tm_min);
>> +    case 3:
>> +        return to_bcd(now.tm_hour);
>
> There is an interesting century bit in specs.

Which I could not figure out how should work and guests seem to be happy 
without it so I did not try to implement it.

>> +    case 4:
>> +        return to_bcd(now.tm_wday);
>> +    case 5:
>> +        return to_bcd(now.tm_mday);
>> +    case 6:
>> +        return to_bcd(now.tm_mon + 1);
>> +    case 7:
>> +        return to_bcd(now.tm_year % 100);
>> +    case 8 ... 19:
>> +        qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
>
> is the beginning \n needed ?

Probably not, maybe left there due to previous debug logs I've removed. 
I'll drop the beginning \n-s in next version.

> Thanks,
>
> C.

Thanks for the review,
BALATON Zoltan

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation

[Cédric Le Goater]

On 06/08/2018 06:16 PM, BALATON Zoltan wrote:
> On Fri, 8 Jun 2018, Cédric Le Goater wrote:
>> On 06/06/2018 03:31 PM, BALATON Zoltan wrote:
>>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>>> of day is implemented. Setting time and RTC alarm are not supported.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  MAINTAINERS                     |   1 +
>>>  default-configs/ppc-softmmu.mak |   1 +
>>>  hw/timer/Makefile.objs          |   1 +
>>>  hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 120 insertions(+)
>>>  create mode 100644 hw/timer/m41t80.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 41cd373..9e13bc1 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
>>>  L: qemu-ppc@nongnu.org
>>>  S: Maintained
>>>  F: hw/ide/sii3112.c
>>> +F: hw/timer/m41t80.c
>>>
>>>  SH4 Machines
>>>  ------------
>>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>>> index 7d0dc2f..9fbaadc 100644
>>> --- a/default-configs/ppc-softmmu.mak
>>> +++ b/default-configs/ppc-softmmu.mak
>>> @@ -27,6 +27,7 @@ CONFIG_SM501=y
>>>  CONFIG_IDE_SII3112=y
>>>  CONFIG_I2C=y
>>>  CONFIG_BITBANG_I2C=y
>>> +CONFIG_M41T80=y
>>>
>>>  # For Macs
>>>  CONFIG_MAC=y
>>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>>> index 8b27a4b..e16b2b9 100644
>>> --- a/hw/timer/Makefile.objs
>>> +++ b/hw/timer/Makefile.objs
>>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
>>>  common-obj-$(CONFIG_DS1338) += ds1338.o
>>>  common-obj-$(CONFIG_HPET) += hpet.o
>>>  common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
>>> +common-obj-$(CONFIG_M41T80) += m41t80.o
>>>  common-obj-$(CONFIG_M48T59) += m48t59.o
>>>  ifeq ($(CONFIG_ISA_BUS),y)
>>>  common-obj-$(CONFIG_M48T59) += m48t59-isa.o
>>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>>> new file mode 100644
>>> index 0000000..9dbdb1b
>>> --- /dev/null
>>> +++ b/hw/timer/m41t80.c
>>> @@ -0,0 +1,117 @@
>>> +/*
>>> + * M41T80 serial rtc emulation
>>> + *
>>> + * Copyright (c) 2018 BALATON Zoltan
>>> + *
>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>> + *
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>> +#include "qemu/timer.h"
>>> +#include "qemu/bcd.h"
>>> +#include "hw/i2c/i2c.h"
>>> +
>>> +#define TYPE_M41T80 "m41t80"
>>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>>> +
>>> +typedef struct M41t80State {
>>> +    I2CSlave parent_obj;
>>> +    int8_t addr;
>>> +} M41t80State;
>>> +
>>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    M41t80State *s = M41T80(dev);
>>> +
>>> +    s->addr = -1;
>>> +}
>>> +
>>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>>> +{
>>> +    M41t80State *s = M41T80(i2c);
>>> +
>>> +    if (s->addr < 0) {
>>> +        s->addr = data;
>>> +    } else {
>>> +        s->addr++;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +static int m41t80_recv(I2CSlave *i2c)
>>> +{
>>> +    M41t80State *s = M41T80(i2c);
>>> +    struct tm now;
>>> +    qemu_timeval tv;
>>> +
>>> +    if (s->addr < 0) {
>>> +        s->addr = 0;
>>> +    }
>>> +    if (s->addr >= 1 && s->addr <= 7) {
>>> +        qemu_get_timedate(&now, -1);
>>> +    }
>>> +    switch (s->addr++) {
>>
>> you could use some define to name the registers :
> 
> This was also suggested by Philippe Mathieu-Daudé and my answer to that was that I don't feel like I want to come up with names the datasheet does not have either. I think this device is simple enough with just 20 consecutively numbered registers that appear only in these switch cases by number as in the datasheet table so that I don't want to make it more difficult to read by encrypting these numbers behind some arbitrary defines without a good reason. They are also so simple that it's clear from the usually one line implementation what they do so that's also not a good reason to name them.

OK. It's fine with me but you might get some inspiration from Linux 
for the names :)

>>> +    case 0:
>>> +        qemu_gettimeofday(&tv);
>>> +        return to_bcd(tv.tv_usec / 10000);> +    case 1:
>>> +        return to_bcd(now.tm_sec);
>>> +    case 2:
>>> +        return to_bcd(now.tm_min);
>>> +    case 3:
>>> +        return to_bcd(now.tm_hour);
>>
>> There is an interesting century bit in specs.
> 
> Which I could not figure out how should work and guests seem to be happy without it so I did not try to implement it.

yes. It seems that Linux simply ignores it. Let's forget it.

Thanks,
C.

>>> +    case 4:
>>> +        return to_bcd(now.tm_wday);
>>> +    case 5:
>>> +        return to_bcd(now.tm_mday);
>>> +    case 6:
>>> +        return to_bcd(now.tm_mon + 1);
>>> +    case 7:
>>> +        return to_bcd(now.tm_year % 100);
>>> +    case 8 ... 19:
>>> +        qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
>>
>> is the beginning \n needed ?
> 
> Probably not, maybe left there due to previous debug logs I've removed. I'll drop the beginning \n-s in next version.
> 
>> Thanks,
>>
>> C.
> 
> Thanks for the review,
> BALATON Zoltan

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

[BALATON Zoltan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 9149 bytes --]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 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 */
-- 
2.7.6

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

[David Gibson]

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

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

[BALATON Zoltan]

On Fri, 8 Jun 2018, David Gibson wrote:
> 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.

Maybe it will be clearer after the next patch in the series. I needed a 
place to store the bitbang_i2c_interface for the directcntl way of 
accessing the i2c bus but I can't include bitbang_i2c.h from the public 
header because it's a local header. So I needed a local extension to the 
state struct. Once I have that then it's a good place to also store 
private registers which are now defined in the same file so I don't have 
to look up them in a different place. This seemed clearer to me and easier 
to work with. Maybe the spliting of the rewrite did not make this clear.

One thing I'm not sure about though:

>> ---
>>  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
[...]
>> @@ -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);

I allocate memory here but I'm not sure if it should be g_free'd somewhere 
and if so where? I was not able to detangle QOM object hierarchies and 
there seems to be no good docs available or I haven't found them. (PCI 
devices seem to have unrealize methods but this did not work for I2C 
objects.)

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

[David Gibson]

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

On Fri, Jun 08, 2018 at 11:20:50AM +0200, BALATON Zoltan wrote:
> On Fri, 8 Jun 2018, David Gibson wrote:
> > 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.
> 
> Maybe it will be clearer after the next patch in the series. I needed a
> place to store the bitbang_i2c_interface for the directcntl way of accessing
> the i2c bus but I can't include bitbang_i2c.h from the public header because
> it's a local header. So I needed a local extension to the state struct. Once
> I have that then it's a good place to also store private registers which are
> now defined in the same file so I don't have to look up them in a different
> place. This seemed clearer to me and easier to work with. Maybe the spliting
> of the rewrite did not make this clear.

Oh.. right.  There's a better way.

You can just forward declare the bitbang_i2c_interface structure like
this in your header:
	typdef struct bitbang_i2c_interface bitbang_i2c_interface;

So you're declaring the existence of the structure, but not its
contents - that's sufficient to create a pointer to it.  Then you
don't need to creat the substructure and extra level of indirection.

> One thing I'm not sure about though:
> 
> > > ---
> > >  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
> [...]
> > > @@ -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);
> 
> I allocate memory here but I'm not sure if it should be g_free'd somewhere
> and if so where? I was not able to detangle QOM object hierarchies and there
> seems to be no good docs available or I haven't found them. (PCI devices
> seem to have unrealize methods but this did not work for I2C objects.)

Yes, if you're allocating you definitely should be free()ing.  It
should go in the corresponding cleanup routine to where it is
allocated.  Since the allocation is in instance_init(), the free()
should be in instance_finalize() (which you'd need to add).

Except that the above should let you avoid that.

..and I guess this won't actually ever be finalized in practice.

..and there doesn't seem to be a way to free up a bitbang_interface,
so even if you added the finalize, it still wouldn't really clean up
properly.

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

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

[BALATON Zoltan]

On Wed, 13 Jun 2018, David Gibson wrote:
> On Fri, Jun 08, 2018 at 11:20:50AM +0200, BALATON Zoltan wrote:
>> On Fri, 8 Jun 2018, David Gibson wrote:
>>> 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.
>>
>> Maybe it will be clearer after the next patch in the series. I needed a
>> place to store the bitbang_i2c_interface for the directcntl way of accessing
>> the i2c bus but I can't include bitbang_i2c.h from the public header because
>> it's a local header. So I needed a local extension to the state struct. Once
>> I have that then it's a good place to also store private registers which are
>> now defined in the same file so I don't have to look up them in a different
>> place. This seemed clearer to me and easier to work with. Maybe the spliting
>> of the rewrite did not make this clear.
>
> Oh.. right.  There's a better way.
>
> You can just forward declare the bitbang_i2c_interface structure like
> this in your header:
> 	typdef struct bitbang_i2c_interface bitbang_i2c_interface;
>
> So you're declaring the existence of the structure, but not its
> contents - that's sufficient to create a pointer to it.  Then you
> don't need to creat the substructure and extra level of indirection.
>
>> One thing I'm not sure about though:
>>
>>>> ---
>>>>  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
>> [...]
>>>> @@ -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);
>>
>> I allocate memory here but I'm not sure if it should be g_free'd somewhere
>> and if so where? I was not able to detangle QOM object hierarchies and there
>> seems to be no good docs available or I haven't found them. (PCI devices
>> seem to have unrealize methods but this did not work for I2C objects.)
>
> Yes, if you're allocating you definitely should be free()ing.  It
> should go in the corresponding cleanup routine to where it is
> allocated.  Since the allocation is in instance_init(), the free()
> should be in instance_finalize() (which you'd need to add).
>
> Except that the above should let you avoid that.
>
> ..and I guess this won't actually ever be finalized in practice.
>
> ..and there doesn't seem to be a way to free up a bitbang_interface,
> so even if you added the finalize, it still wouldn't really clean up
> properly.

Yes, I suspected it won't matter anyway. I'll try your suggestion to just 
declare the bitbang_i2c_interface in the public header in next version.

Any more reviews to expect from you for other patches or should I send a 
v3 with the changes so far?

Thank you,
BALATON Zoltan

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

[David Gibson]

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

On Wed, Jun 13, 2018 at 10:56:59AM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Fri, Jun 08, 2018 at 11:20:50AM +0200, BALATON Zoltan wrote:
> > > On Fri, 8 Jun 2018, David Gibson wrote:
> > > > 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.
> > > 
> > > Maybe it will be clearer after the next patch in the series. I needed a
> > > place to store the bitbang_i2c_interface for the directcntl way of accessing
> > > the i2c bus but I can't include bitbang_i2c.h from the public header because
> > > it's a local header. So I needed a local extension to the state struct. Once
> > > I have that then it's a good place to also store private registers which are
> > > now defined in the same file so I don't have to look up them in a different
> > > place. This seemed clearer to me and easier to work with. Maybe the spliting
> > > of the rewrite did not make this clear.
> > 
> > Oh.. right.  There's a better way.
> > 
> > You can just forward declare the bitbang_i2c_interface structure like
> > this in your header:
> > 	typdef struct bitbang_i2c_interface bitbang_i2c_interface;
> > 
> > So you're declaring the existence of the structure, but not its
> > contents - that's sufficient to create a pointer to it.  Then you
> > don't need to creat the substructure and extra level of indirection.
> > 
> > > One thing I'm not sure about though:
> > > 
> > > > > ---
> > > > >  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
> > > [...]
> > > > > @@ -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);
> > > 
> > > I allocate memory here but I'm not sure if it should be g_free'd somewhere
> > > and if so where? I was not able to detangle QOM object hierarchies and there
> > > seems to be no good docs available or I haven't found them. (PCI devices
> > > seem to have unrealize methods but this did not work for I2C objects.)
> > 
> > Yes, if you're allocating you definitely should be free()ing.  It
> > should go in the corresponding cleanup routine to where it is
> > allocated.  Since the allocation is in instance_init(), the free()
> > should be in instance_finalize() (which you'd need to add).
> > 
> > Except that the above should let you avoid that.
> > 
> > ..and I guess this won't actually ever be finalized in practice.
> > 
> > ..and there doesn't seem to be a way to free up a bitbang_interface,
> > so even if you added the finalize, it still wouldn't really clean up
> > properly.
> 
> Yes, I suspected it won't matter anyway. I'll try your suggestion to just
> declare the bitbang_i2c_interface in the public header in next version.
> 
> Any more reviews to expect from you for other patches or should I send a v3
> with the changes so far?

Go ahead with v3.

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

[Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging

[BALATON Zoltan]

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
 
 #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);
         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;
     }
 }
-- 
2.7.6

Re: [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging

[Philippe Mathieu-Daudé]

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;
>      }
>  }
> 

Re: [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging

[David Gibson]

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

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.

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

Re: [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging

[BALATON Zoltan]

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.

[Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register

[BALATON Zoltan]

When writing a register that has read only bits besides reserved bits
we have to avoid changing read only bits that may have non zero
default values.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index e47be99..7ec1434 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -836,10 +836,10 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
 
     switch (addr) {
     case SM501_SYSTEM_CONTROL:
-        s->system_control = value & 0xE300B8F7;
+        s->system_control |= value & 0xEF00B8F7;
         break;
     case SM501_MISC_CONTROL:
-        s->misc_control = value & 0xFF7FFF20;
+        s->misc_control |= value & 0xFF7FFF10;
         break;
     case SM501_GPIO31_0_CONTROL:
         s->gpio_31_0_control = value;
@@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
         s->dram_control |=  value & 0x7FFFFFC3;
         break;
     case SM501_ARBTRTN_CONTROL:
-        s->arbitration_control =  value & 0x37777777;
+        s->arbitration_control = value & 0x37777777;
         break;
     case SM501_IRQ_MASK:
         s->irq_mask = value;
-- 
2.7.6

Re: [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register

[Peter Maydell]

On 6 June 2018 at 14:31, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> When writing a register that has read only bits besides reserved bits
> we have to avoid changing read only bits that may have non zero
> default values.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/sm501.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index e47be99..7ec1434 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -836,10 +836,10 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>
>      switch (addr) {
>      case SM501_SYSTEM_CONTROL:
> -        s->system_control = value & 0xE300B8F7;
> +        s->system_control |= value & 0xEF00B8F7;

This will mean that you can't clear a r/w bit by writing a
zero to it -- is that the hardware behaviour? The
description in the commit message suggests that you want
   s->whatever = (value & rw_bit_mask) | ro_one_bits_mask;

?

>          break;
>      case SM501_MISC_CONTROL:
> -        s->misc_control = value & 0xFF7FFF20;
> +        s->misc_control |= value & 0xFF7FFF10;
>          break;
>      case SM501_GPIO31_0_CONTROL:
>          s->gpio_31_0_control = value;
> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>          s->dram_control |=  value & 0x7FFFFFC3;
>          break;
>      case SM501_ARBTRTN_CONTROL:
> -        s->arbitration_control =  value & 0x37777777;
> +        s->arbitration_control = value & 0x37777777;

Was this intended to be changed too?

>          break;
>      case SM501_IRQ_MASK:
>          s->irq_mask = value;

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register

[BALATON Zoltan]

On Wed, 6 Jun 2018, Peter Maydell wrote:
> On 6 June 2018 at 14:31, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> When writing a register that has read only bits besides reserved bits
>> we have to avoid changing read only bits that may have non zero
>> default values.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/display/sm501.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index e47be99..7ec1434 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -836,10 +836,10 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>>
>>      switch (addr) {
>>      case SM501_SYSTEM_CONTROL:
>> -        s->system_control = value & 0xE300B8F7;
>> +        s->system_control |= value & 0xEF00B8F7;
>
> This will mean that you can't clear a r/w bit by writing a
> zero to it -- is that the hardware behaviour? The

Not really sure about real hardware behaviour. I only have that not very 
detailed docs we've looked at before but I assume r/w bits could be 
changed by writing them.

> description in the commit message suggests that you want
>   s->whatever = (value & rw_bit_mask) | ro_one_bits_mask;

Wouldn't that always set ro bits? I need to leave ro bits untouched by 
written value and only set rw bits. The previous version masked out ro 
bits which also cleared them due to assigning with =. Maybe

s->whatever &= ro_bits_mask;
s->whatever |= value & rw_bits_mask;

would work? Could this be simplified?

>>          break;
>>      case SM501_MISC_CONTROL:
>> -        s->misc_control = value & 0xFF7FFF20;
>> +        s->misc_control |= value & 0xFF7FFF10;
>>          break;
>>      case SM501_GPIO31_0_CONTROL:
>>          s->gpio_31_0_control = value;
>> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>>          s->dram_control |=  value & 0x7FFFFFC3;
>>          break;
>>      case SM501_ARBTRTN_CONTROL:
>> -        s->arbitration_control =  value & 0x37777777;
>> +        s->arbitration_control = value & 0x37777777;
>
> Was this intended to be changed too?

Yes, just a simple white space cleanup. Does that worth a separate patch 
or enough to mention in commit message to make it clear it's intended 
change?

Thanks for the quick review,
BALATON Zoltan

Re: [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register

[Peter Maydell]

On 6 June 2018 at 15:28, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Wed, 6 Jun 2018, Peter Maydell wrote:
>>
>> On 6 June 2018 at 14:31, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>
>>> When writing a register that has read only bits besides reserved bits
>>> we have to avoid changing read only bits that may have non zero
>>> default values.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/display/sm501.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>> index e47be99..7ec1434 100644
>>> --- a/hw/display/sm501.c
>>> +++ b/hw/display/sm501.c
>>> @@ -836,10 +836,10 @@ static void sm501_system_config_write(void *opaque,
>>> hwaddr addr,
>>>
>>>      switch (addr) {
>>>      case SM501_SYSTEM_CONTROL:
>>> -        s->system_control = value & 0xE300B8F7;
>>> +        s->system_control |= value & 0xEF00B8F7;
>>
>>
>> This will mean that you can't clear a r/w bit by writing a
>> zero to it -- is that the hardware behaviour? The
>
>
> Not really sure about real hardware behaviour. I only have that not very
> detailed docs we've looked at before but I assume r/w bits could be changed
> by writing them.
>
>> description in the commit message suggests that you want
>>   s->whatever = (value & rw_bit_mask) | ro_one_bits_mask;
>
>
> Wouldn't that always set ro bits?

It will enforce that the RO 1 bits are always 1 (by
ORing those bits in). It's not the only way to do it.
(NB we OR the "ro_one_bits", not "ro_bits".)

>I need to leave ro bits untouched by
> written value and only set rw bits. The previous version masked out ro bits
> which also cleared them due to assigning with =. Maybe
>
> s->whatever &= ro_bits_mask;
> s->whatever |= value & rw_bits_mask;

Yes, this works (and I have a feeling it's what we tend to use.)

>>> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque,
>>> hwaddr addr,
>>>          s->dram_control |=  value & 0x7FFFFFC3;

Hmm, does dram_control also need to behave the same way?

>>>          break;
>>>      case SM501_ARBTRTN_CONTROL:
>>> -        s->arbitration_control =  value & 0x37777777;
>>> +        s->arbitration_control = value & 0x37777777;
>>
>>
>> Was this intended to be changed too?
>
>
> Yes, just a simple white space cleanup. Does that worth a separate patch or
> enough to mention in commit message to make it clear it's intended change?

I would personally put it in its own patch, but if you put it
in this one do mention it in the commit message. Otherwise
given the patch is changing four 'foo_control' registers it
looks like this one was typoed. (It will look less so if
the change for the others isn't just "add a '|'", though.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register

[BALATON Zoltan]

On Wed, 6 Jun 2018, Peter Maydell wrote:
> On 6 June 2018 at 15:28, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> s->whatever &= ro_bits_mask;
>> s->whatever |= value & rw_bits_mask;
>
> Yes, this works (and I have a feeling it's what we tend to use.)

OK, I'll do this then in next respin.

>>>> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque,
>>>> hwaddr addr,
>>>>          s->dram_control |=  value & 0x7FFFFFC3;
>
> Hmm, does dram_control also need to behave the same way?

It has one read only bit which we are not emulating so maybe not 
significant but changed it as well for correctness and added some more 
similar registers as well.

>>>>          break;
>>>>      case SM501_ARBTRTN_CONTROL:
>>>> -        s->arbitration_control =  value & 0x37777777;
>>>> +        s->arbitration_control = value & 0x37777777;
>>>
>>>
>>> Was this intended to be changed too?
>>
>>
>> Yes, just a simple white space cleanup. Does that worth a separate patch or
>> enough to mention in commit message to make it clear it's intended change?
>
> I would personally put it in its own patch, but if you put it
> in this one do mention it in the commit message. Otherwise
> given the patch is changing four 'foo_control' registers it
> looks like this one was typoed. (It will look less so if
> the change for the others isn't just "add a '|'", though.)

I've left it in one patch to avoid having two many patches in the series 
but added it to commit message together with dram_control which also had 
a whitespace removed in same patch.

Before sending v3 I'm waiting if there are any more comments for other 
patches in this series 
(http://patchew.org/QEMU/cover.1528291908.git.balaton@eik.bme.hu/) but 
looks like David is busy and could not look at them yet. Not sure if 
you've seen these patches but if you have a moment your comments are 
welcome. If no other changes are suggested I'll submit v3 in the coming 
days.

Thank you,
BALATON Zoltan

[Qemu-devel] [PATCH v2 8/8] sm501: Implement i2c part for reading monitor EDID

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 default-configs/ppc-softmmu.mak    |   1 +
 default-configs/ppcemb-softmmu.mak |   1 +
 default-configs/sh4-softmmu.mak    |   1 +
 default-configs/sh4eb-softmmu.mak  |   1 +
 hw/display/sm501.c                 | 136 +++++++++++++++++++++++++++++++++++--
 5 files changed, 136 insertions(+), 4 deletions(-)

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 9fbaadc..860de80 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -24,6 +24,7 @@ CONFIG_ETSEC=y
 # For Sam460ex
 CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index 37af193..ac44f15 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -17,6 +17,7 @@ CONFIG_XILINX=y
 CONFIG_XILINX_ETHLITE=y
 CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
diff --git a/default-configs/sh4-softmmu.mak b/default-configs/sh4-softmmu.mak
index 546d855..72d8fca 100644
--- a/default-configs/sh4-softmmu.mak
+++ b/default-configs/sh4-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
 CONFIG_SH4=y
 CONFIG_IDE_MMIO=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_ISA_TESTDEV=y
 CONFIG_I82378=y
 CONFIG_I8259=y
diff --git a/default-configs/sh4eb-softmmu.mak b/default-configs/sh4eb-softmmu.mak
index 2d3fd49..c686637 100644
--- a/default-configs/sh4eb-softmmu.mak
+++ b/default-configs/sh4eb-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
 CONFIG_SH4=y
 CONFIG_IDE_MMIO=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_ISA_TESTDEV=y
 CONFIG_I82378=y
 CONFIG_I8259=y
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 7ec1434..d357583 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 #include "qemu-common.h"
 #include "cpu.h"
 #include "hw/hw.h"
@@ -34,6 +35,8 @@
 #include "hw/devices.h"
 #include "hw/sysbus.h"
 #include "hw/pci/pci.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/i2c-ddc.h"
 #include "qemu/range.h"
 #include "ui/pixel_ops.h"
 
@@ -471,10 +474,12 @@ typedef struct SM501State {
     MemoryRegion local_mem_region;
     MemoryRegion mmio_region;
     MemoryRegion system_config_region;
+    MemoryRegion i2c_region;
     MemoryRegion disp_ctrl_region;
     MemoryRegion twoD_engine_region;
     uint32_t last_width;
     uint32_t last_height;
+    I2CBus *i2c_bus;
 
     /* mmio registers */
     uint32_t system_control;
@@ -487,6 +492,11 @@ typedef struct SM501State {
     uint32_t misc_timing;
     uint32_t power_mode_control;
 
+    uint8_t i2c_byte_count;
+    uint8_t i2c_status;
+    uint8_t i2c_addr;
+    uint8_t i2c_data[16];
+
     uint32_t uart0_ier;
     uint32_t uart0_lcr;
     uint32_t uart0_mcr;
@@ -894,6 +904,107 @@ static const MemoryRegionOps sm501_system_config_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static uint64_t sm501_i2c_read(void *opaque, hwaddr addr, unsigned size)
+{
+    SM501State *s = (SM501State *)opaque;
+    uint8_t ret = 0;
+
+    switch (addr) {
+    case SM501_I2C_BYTE_COUNT:
+        ret = s->i2c_byte_count;
+        break;
+    case SM501_I2C_STATUS:
+        ret = s->i2c_status;
+        break;
+    case SM501_I2C_SLAVE_ADDRESS:
+        ret = s->i2c_addr;
+        break;
+    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
+        ret = s->i2c_data[addr - SM501_I2C_DATA];
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register read."
+                      " addr=0x%" HWADDR_PRIx "\n", addr);
+    }
+
+    SM501_DPRINTF("sm501 i2c regs : read addr=%" HWADDR_PRIx " val=%x\n",
+                  addr, ret);
+    return ret;
+}
+
+static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
+                            unsigned size)
+{
+    SM501State *s = (SM501State *)opaque;
+    SM501_DPRINTF("sm501 i2c regs : write addr=%" HWADDR_PRIx
+                  " val=%" PRIx64 "\n", addr, value);
+
+    switch (addr) {
+    case SM501_I2C_BYTE_COUNT:
+        s->i2c_byte_count = value & 0xf;
+        break;
+    case SM501_I2C_CONTROL:
+        if (value & 1) {
+            if (value & 4) {
+                int res = i2c_start_transfer(s->i2c_bus,
+                                             s->i2c_addr >> 1,
+                                             s->i2c_addr & 1);
+                s->i2c_status |= (res ? 1 << 2 : 0);
+                if (!res) {
+                    int i;
+                    SM501_DPRINTF("sm501 i2c : transferring %d bytes to 0x%x\n",
+                                  s->i2c_byte_count + 1, s->i2c_addr >> 1);
+                    for (i = 0; i <= s->i2c_byte_count; i++) {
+                        res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
+                                            !(s->i2c_addr & 1));
+                        if (res) {
+                            SM501_DPRINTF("sm501 i2c : transfer failed"
+                                          " i=%d, res=%d\n", i, res);
+                            s->i2c_status |= (res ? 1 << 2 : 0);
+                            return;
+                        }
+                    }
+                    if (i) {
+                        SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
+                        s->i2c_status = 8;
+                    }
+                }
+            } else {
+                SM501_DPRINTF("sm501 i2c : end transfer\n");
+                i2c_end_transfer(s->i2c_bus);
+                s->i2c_status &= ~4;
+            }
+        }
+        break;
+    case SM501_I2C_RESET:
+            s->i2c_status &= ~4;
+        break;
+    case SM501_I2C_SLAVE_ADDRESS:
+        s->i2c_addr = value & 0xff;
+        break;
+    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
+        s->i2c_data[addr - SM501_I2C_DATA] = value & 0xff;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register write. "
+                      "addr=0x%" HWADDR_PRIx " val=%" PRIx64 "\n", addr, value);
+    }
+}
+
+static const MemoryRegionOps sm501_i2c_ops = {
+    .read = sm501_i2c_read,
+    .write = sm501_i2c_write,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
 static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
 {
     SM501State *s = (SM501State *)opaque;
@@ -1574,6 +1685,10 @@ static void sm501_reset(SM501State *s)
     s->irq_mask = 0;
     s->misc_timing = 0;
     s->power_mode_control = 0;
+    s->i2c_byte_count = 0;
+    s->i2c_status = 0;
+    s->i2c_addr = 0;
+    memset(s->i2c_data, 0, 16);
     s->dc_panel_control = 0x00010000; /* FIFO level 3 */
     s->dc_video_control = 0;
     s->dc_crt_control = 0x00010000;
@@ -1612,6 +1727,11 @@ static void sm501_init(SM501State *s, DeviceState *dev,
     memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
     s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
 
+    /* i2c */
+    s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");
+    I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
+    i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
+
     /* mmio */
     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
     memory_region_init_io(&s->system_config_region, OBJECT(dev),
@@ -1619,6 +1739,9 @@ static void sm501_init(SM501State *s, DeviceState *dev,
                           "sm501-system-config", 0x6c);
     memory_region_add_subregion(&s->mmio_region, SM501_SYS_CONFIG,
                                 &s->system_config_region);
+    memory_region_init_io(&s->i2c_region, OBJECT(dev), &sm501_i2c_ops, s,
+                          "sm501-i2c", 0x14);
+    memory_region_add_subregion(&s->mmio_region, SM501_I2C, &s->i2c_region);
     memory_region_init_io(&s->disp_ctrl_region, OBJECT(dev),
                           &sm501_disp_ctrl_ops, s,
                           "sm501-disp-ctrl", 0x1000);
@@ -1702,6 +1825,11 @@ static const VMStateDescription vmstate_sm501_state = {
         VMSTATE_UINT32(twoD_destination_base, SM501State),
         VMSTATE_UINT32(twoD_alpha, SM501State),
         VMSTATE_UINT32(twoD_wrap, SM501State),
+        /* Added in version 2 */
+        VMSTATE_UINT8(i2c_byte_count, SM501State),
+        VMSTATE_UINT8(i2c_status, SM501State),
+        VMSTATE_UINT8(i2c_addr, SM501State),
+        VMSTATE_UINT8_ARRAY(i2c_data, SM501State, 16),
         VMSTATE_END_OF_LIST()
      }
 };
@@ -1767,8 +1895,8 @@ static void sm501_reset_sysbus(DeviceState *dev)
 
 static const VMStateDescription vmstate_sm501_sysbus = {
     .name = TYPE_SYSBUS_SM501,
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(state, SM501SysBusState, 1,
                        vmstate_sm501_state, SM501State),
@@ -1840,8 +1968,8 @@ static void sm501_reset_pci(DeviceState *dev)
 
 static const VMStateDescription vmstate_sm501_pci = {
     .name = TYPE_PCI_SM501,
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
         VMSTATE_STRUCT(state, SM501PCIState, 1,
-- 
2.7.6

[Qemu-devel] [PATCH v2 4/8] ppc4xx_i2c: Rewrite to model hardware more closely

[BALATON Zoltan]

Rewrite to make it closer to how real device works so that guest OS
drivers can access I2C devices. Previously this was only a hack to
allow U-Boot to get past accessing SPD EEPROMs but to support other
I2C devices and allow guests to access them we need to model real
device more properly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
Can't split this up more but since previous version really only worked
with U-Boot and this version still works with that what else could break?
This version however also works with AROS, Linux and AmigaOS. So it's
an improvement and if something else breaks due to this then that's a
bug that should be fixed separately instead of this patch being dropped.

hw/i2c/ppc4xx_i2c.c | 223 +++++++++++++++++++++++++---------------------------
 1 file changed, 109 insertions(+), 114 deletions(-)

diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index 5806209..8c381ec 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -38,13 +38,26 @@
 #define IIC_CNTL_READ       (1 << 1)
 #define IIC_CNTL_CHT        (1 << 2)
 #define IIC_CNTL_RPST       (1 << 3)
+#define IIC_CNTL_AMD        (1 << 6)
+#define IIC_CNTL_HMT        (1 << 7)
+
+#define IIC_MDCNTL_EINT     (1 << 2)
+#define IIC_MDCNTL_ESM      (1 << 3)
+#define IIC_MDCNTL_FMDB     (1 << 6)
 
 #define IIC_STS_PT          (1 << 0)
+#define IIC_STS_IRQA        (1 << 1)
 #define IIC_STS_ERR         (1 << 2)
+#define IIC_STS_MDBF        (1 << 4)
 #define IIC_STS_MDBS        (1 << 5)
 
 #define IIC_EXTSTS_XFRA     (1 << 0)
 
+#define IIC_INTRMSK_EIMTC   (1 << 0)
+#define IIC_INTRMSK_EITA    (1 << 1)
+#define IIC_INTRMSK_EIIC    (1 << 2)
+#define IIC_INTRMSK_EIHE    (1 << 3)
+
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
 #define IIC_DIRECTCNTL_SDAC (1 << 3)
@@ -54,7 +67,8 @@
 
 typedef struct {
     bitbang_i2c_interface *bitbang;
-    uint8_t mdata;
+    int mdidx;
+    uint8_t mdata[4];
     uint8_t lmadr;
     uint8_t hmadr;
     uint8_t cntl;
@@ -74,21 +88,13 @@ static void ppc4xx_i2c_reset(DeviceState *s)
 {
     PPC4xxI2CRegs *i2c = PPC4xx_I2C(s)->regs;
 
-    /* FIXME: Should also reset bus?
-     *if (s->address != ADDR_RESET) {
-     *    i2c_end_transfer(s->bus);
-     *}
-     */
-
-    i2c->mdata = 0;
-    i2c->lmadr = 0;
-    i2c->hmadr = 0;
+    i2c->mdidx = -1;
+    memset(i2c->mdata, 0, ARRAY_SIZE(i2c->mdata));
+    /* [hl][ms]addr are not affected by reset */
     i2c->cntl = 0;
     i2c->mdcntl = 0;
     i2c->sts = 0;
-    i2c->extsts = 0x8f;
-    i2c->lsadr = 0;
-    i2c->hsadr = 0;
+    i2c->extsts = (1 << 6);
     i2c->clkdiv = 0;
     i2c->intrmsk = 0;
     i2c->xfrcnt = 0;
@@ -96,70 +102,30 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->directcntl = 0xf;
 }
 
-static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
-{
-    return true;
-}
-
 static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
 {
     PPC4xxI2CState *s = PPC4xx_I2C(opaque);
     PPC4xxI2CRegs *i2c = s->regs;
     uint64_t ret;
+    int i;
 
     switch (addr) {
     case 0:
-        ret = i2c->mdata;
-        if (ppc4xx_i2c_is_master(s)) {
+        if (i2c->mdidx < 0) {
             ret = 0xff;
-
-            if (!(i2c->sts & IIC_STS_MDBS)) {
-                qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read "
-                              "without starting transfer\n",
-                              TYPE_PPC4xx_I2C, __func__);
-            } else {
-                int pending = (i2c->cntl >> 4) & 3;
-
-                /* get the next byte */
-                int byte = i2c_recv(s->bus);
-
-                if (byte < 0) {
-                    qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed "
-                                  "for device 0x%02x\n", TYPE_PPC4xx_I2C,
-                                  __func__, i2c->lmadr);
-                    ret = 0xff;
-                } else {
-                    ret = byte;
-                    /* Raise interrupt if enabled */
-                    /*ppc4xx_i2c_raise_interrupt(i2c)*/;
-                }
-
-                if (!pending) {
-                    i2c->sts &= ~IIC_STS_MDBS;
-                    /*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(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;
-                        i2c->extsts |= IIC_EXTSTS_XFRA;
-                    } else {
-                        /*i2c->sts |= IIC_STS_PT;*/
-                        i2c->sts |= IIC_STS_MDBS;
-                        i2c->sts &= ~IIC_STS_ERR;
-                        i2c->extsts = 0;
-                    }
-                }
-                pending--;
-                i2c->cntl = (i2c->cntl & 0xcf) | (pending << 4);
-            }
-        } else {
-            qemu_log_mask(LOG_UNIMP, "[%s]%s: slave mode not implemented\n",
-                          TYPE_PPC4xx_I2C, __func__);
+            break;
+        }
+        ret = i2c->mdata[0];
+        if (i2c->mdidx == 3) {
+            i2c->sts &= ~IIC_STS_MDBF;
+        } else if (i2c->mdidx == 0) {
+            i2c->sts &= ~IIC_STS_MDBS;
+        }
+        for (i = 0; i < i2c->mdidx; i++) {
+            i2c->mdata[i] = i2c->mdata[i + 1];
+        }
+        if (i2c->mdidx >= 0) {
+            i2c->mdidx--;
         }
         break;
     case 4:
@@ -179,6 +145,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
         break;
     case 9:
         ret = i2c->extsts;
+        ret |= !!i2c_bus_busy(s->bus) << 4;
         break;
     case 10:
         ret = i2c->lsadr;
@@ -223,70 +190,98 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
 
     switch (addr) {
     case 0:
-        i2c->mdata = value;
-        if (!i2c_bus_busy(s->bus)) {
-            /* assume we start a write transfer */
-            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;
-                i2c->extsts |= IIC_EXTSTS_XFRA;
-            } else {
-                i2c->sts |= IIC_STS_PT;
-                i2c->sts &= ~IIC_STS_ERR;
-                i2c->extsts = 0;
-            }
+        if (i2c->mdidx >= 3) {
+            break;
         }
-        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(s->bus);
-            }
+        i2c->mdata[++i2c->mdidx] = value;
+        if (i2c->mdidx == 3) {
+            i2c->sts |= IIC_STS_MDBF;
+        } else if (i2c->mdidx == 0) {
+            i2c->sts |= IIC_STS_MDBS;
         }
         break;
     case 4:
         i2c->lmadr = value;
-        if (i2c_bus_busy(s->bus)) {
-            i2c_end_transfer(s->bus);
-        }
         break;
     case 5:
         i2c->hmadr = value;
         break;
     case 6:
-        i2c->cntl = value;
-        if (i2c->cntl & IIC_CNTL_PT) {
-            if (i2c->cntl & IIC_CNTL_READ) {
-                if (i2c_bus_busy(s->bus)) {
-                    /* end previous transfer */
-                    i2c->sts &= ~IIC_STS_PT;
-                    i2c_end_transfer(s->bus);
+        i2c->cntl = value & 0xfe;
+        if (value & IIC_CNTL_AMD) {
+            qemu_log_mask(LOG_UNIMP, "%s: only 7 bit addresses supported\n",
+                          __func__);
+        }
+        if (value & IIC_CNTL_HMT && i2c_bus_busy(s->bus)) {
+            i2c_end_transfer(s->bus);
+            if (i2c->mdcntl & IIC_MDCNTL_EINT &&
+                i2c->intrmsk & IIC_INTRMSK_EIHE) {
+                    i2c->sts |= IIC_STS_IRQA;
+                    qemu_irq_raise(s->irq);
+            }
+        } else if (value & IIC_CNTL_PT) {
+            int recv = (value & IIC_CNTL_READ) >> 1;
+            int tct = value >> 4 & 3;
+            int i;
+
+            if (recv && (i2c->lmadr >> 1) >= 0x50 && (i2c->lmadr >> 1) < 0x58) {
+                /* smbus emulation does not like multi byte reads w/o restart */
+                value |= IIC_CNTL_RPST;
+            }
+
+            for (i = 0; i <= tct; i++) {
+                if (!i2c_bus_busy(s->bus)) {
+                    i2c->extsts = (1 << 6);
+                    if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, recv)) {
+                        i2c->sts |= IIC_STS_ERR;
+                        i2c->extsts |= IIC_EXTSTS_XFRA;
+                        break;
+                    } else {
+                        i2c->sts &= ~IIC_STS_ERR;
+                    }
+                }
+                if (!(i2c->sts & IIC_STS_ERR) &&
+                    i2c_send_recv(s->bus, &i2c->mdata[i], !recv)) {
+                        i2c->sts |= IIC_STS_ERR;
+                        i2c->extsts |= IIC_EXTSTS_XFRA;
+                        break;
                 }
-                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;
-                    i2c->extsts |= IIC_EXTSTS_XFRA;
-                } else {
-                    /*i2c->sts |= IIC_STS_PT;*/
-                    i2c->sts |= IIC_STS_MDBS;
-                    i2c->sts &= ~IIC_STS_ERR;
-                    i2c->extsts = 0;
+                if (value & IIC_CNTL_RPST || !(value & IIC_CNTL_CHT)) {
+                    i2c_end_transfer(s->bus);
                 }
-            } else {
-                /* we actually already did the write transfer... */
-                i2c->sts &= ~IIC_STS_PT;
+            }
+            i2c->xfrcnt = i;
+            i2c->mdidx = i - 1;
+            if (recv && i2c->mdidx >= 0) {
+                i2c->sts |= IIC_STS_MDBS;
+            }
+            if (recv && i2c->mdidx == 3) {
+                i2c->sts |= IIC_STS_MDBF;
+            }
+            if (i && i2c->mdcntl & IIC_MDCNTL_EINT &&
+                i2c->intrmsk & IIC_INTRMSK_EIMTC) {
+                i2c->sts |= IIC_STS_IRQA;
+                qemu_irq_raise(s->irq);
             }
         }
         break;
     case 7:
-        i2c->mdcntl = value & 0xdf;
+        i2c->mdcntl = value & 0x3d;
+        if (value & IIC_MDCNTL_ESM) {
+            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
+                          __func__);
+        }
+        if (value & IIC_MDCNTL_FMDB) {
+            i2c->mdidx = -1;
+            memset(i2c->mdata, 0, ARRAY_SIZE(i2c->mdata));
+            i2c->sts &= ~(IIC_STS_MDBF | IIC_STS_MDBS);
+        }
         break;
     case 8:
         i2c->sts &= ~(value & 0xa);
+        if (value & IIC_STS_IRQA && i2c->mdcntl & IIC_MDCNTL_EINT) {
+            qemu_irq_lower(s->irq);
+        }
         break;
     case 9:
         i2c->extsts &= ~(value & 0x8f);
@@ -307,12 +302,12 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->xfrcnt = value & 0x77;
         break;
     case 15:
+        i2c->xtcntlss &= ~(value & 0xf0);
         if (value & IIC_XTCNTLSS_SRST) {
             /* Is it actually a full reset? U-Boot sets some regs before */
             ppc4xx_i2c_reset(DEVICE(s));
             break;
         }
-        i2c->xtcntlss = value;
         break;
     case 16:
         i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
-- 
2.7.6