[Qemu-devel] [PATCH v3 0/9] Misc sam460ex improvements

[Qemu-devel] [PATCH v3 0/9] Misc sam460ex improvements

[BALATON Zoltan]

This is v3 of sam460ex improvements addressing review comments and
adding two additional patches that allow AmigaOS to progress further
(does not boot fully yet).

BALATON Zoltan (8):
  ppc4xx_i2c: 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 registers
  sm501: Implement i2c part for reading monitor EDID
  target/ppc: Add missing opcode for icbt on PPC440

Sebastian Bauer (1):
  sm501: Perform a full update after palette change

 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                 | 159 +++++++++++++++++++++--
 hw/i2c/ppc4xx_i2c.c                | 251 ++++++++++++++++++-------------------
 hw/ppc/sam460ex.c                  |   1 +
 hw/timer/Makefile.objs             |   1 +
 hw/timer/m41t80.c                  | 117 +++++++++++++++++
 include/hw/i2c/ppc4xx_i2c.h        |  11 +-
 target/ppc/translate.c             |   2 +
 12 files changed, 406 insertions(+), 144 deletions(-)
 create mode 100644 hw/timer/m41t80.c

-- 
2.7.6

[Qemu-devel] [PATCH v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

[BALATON Zoltan]

PPC440 has two opcodes for icbt, add the missing one.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 target/ppc/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 5fe1ba6..3a215a1 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
 GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
 GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
                PPC_BOOKE, PPC2_BOOKE206),
+GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
+               PPC_440_SPEC),
 GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
-- 
2.7.6

Re: [Qemu-devel] [PATCH v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

[David Gibson]

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

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> PPC440 has two opcodes for icbt, add the missing one.

A document reference to confim this would be nice.

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  target/ppc/translate.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 5fe1ba6..3a215a1 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
>  GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
>  GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
>                 PPC_BOOKE, PPC2_BOOKE206),
> +GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
> +               PPC_440_SPEC),
>  GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
>  GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
>  GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),

-- 
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 v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

[BALATON Zoltan]

On Thu, 14 Jun 2018, David Gibson wrote:
> On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:

Maybe amend commit message like this:

According to PPC440 User Manual
>> PPC440 has two opcodes for icbt, add the missing one.

If you can do this when committing then please feel free to adjust this 
commit message as necessary, otherwise I'll change it in next iteration.

Regards,
BALATON Zoltan

> A document reference to confim this would be nice.
>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  target/ppc/translate.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 5fe1ba6..3a215a1 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
>>  GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
>>  GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
>>                 PPC_BOOKE, PPC2_BOOKE206),
>> +GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
>> +               PPC_440_SPEC),
>>  GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
>>  GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
>>  GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
>
>

Re: [Qemu-devel] [PATCH v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

[David Gibson]

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

On Thu, Jun 14, 2018 at 10:03:41AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> 
> Maybe amend commit message like this:
> 
> According to PPC440 User Manual

On which page?  Where can I get that manual?

> > > PPC440 has two opcodes for icbt, add the missing one.
> 
> If you can do this when committing then please feel free to adjust this
> commit message as necessary, otherwise I'll change it in next iteration.
> 
> Regards,
> BALATON Zoltan
> 
> > A document reference to confim this would be nice.
> > 
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > ---
> > >  target/ppc/translate.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > index 5fe1ba6..3a215a1 100644
> > > --- a/target/ppc/translate.c
> > > +++ b/target/ppc/translate.c
> > > @@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
> > >  GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
> > >  GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
> > >                 PPC_BOOKE, PPC2_BOOKE206),
> > > +GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
> > > +               PPC_440_SPEC),
> > >  GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
> > >  GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
> > >  GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
> > 
> > 
> 

-- 
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 v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

[BALATON Zoltan]

On Thu, 14 Jun 2018, David Gibson wrote:
> On Thu, Jun 14, 2018 at 10:03:41AM +0200, BALATON Zoltan wrote:
>> On Thu, 14 Jun 2018, David Gibson wrote:
>>> On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
>>
>> Maybe amend commit message like this:
>>
>> According to PPC440 User Manual
>
> On which page?  Where can I get that manual?

By searching for "PPC440 User Manual"? The one I've found had an opcode 
table at the end in an appendix but I've seen one guest using this machine 
code and get invalid instruction on QEMU while apparently it works on real 
hardware (but I can't test that myself as I don't have real hardware).

Regards,
BALATON Zoltan

>>>> PPC440 has two opcodes for icbt, add the missing one.
>>
>> If you can do this when committing then please feel free to adjust this
>> commit message as necessary, otherwise I'll change it in next iteration.
>>
>> Regards,
>> BALATON Zoltan
>>
>>> A document reference to confim this would be nice.
>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  target/ppc/translate.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>>> index 5fe1ba6..3a215a1 100644
>>>> --- a/target/ppc/translate.c
>>>> +++ b/target/ppc/translate.c
>>>> @@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
>>>>  GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
>>>>  GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
>>>>                 PPC_BOOKE, PPC2_BOOKE206),
>>>> +GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
>>>> +               PPC_440_SPEC),
>>>>  GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
>>>>  GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
>>>>  GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
>>>
>>>
>>
>
>

Re: [Qemu-devel] [PATCH v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

[David Gibson]

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

On Fri, Jun 15, 2018 at 11:35:37AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Thu, Jun 14, 2018 at 10:03:41AM +0200, BALATON Zoltan wrote:
> > > On Thu, 14 Jun 2018, David Gibson wrote:
> > > > On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> > > 
> > > Maybe amend commit message like this:
> > > 
> > > According to PPC440 User Manual
> > 
> > On which page?  Where can I get that manual?
> 
> By searching for "PPC440 User Manual"? The one I've found had an opcode
> table at the end in an appendix but I've seen one guest using this machine
> code and get invalid instruction on QEMU while apparently it works on real
> hardware (but I can't test that myself as I don't have real
> hardware).

Ok, I'm looking for something like this.

| According to the PPC440 User Manual[0] page XXXX, PPC440 has two
| opcodes for icbt, but qemu currently only implements one of them.
| Add the missing one.
|
| [0] https://link/to/the/manual (if you can find one)


> 
> Regards,
> BALATON Zoltan
> 
> > > > > PPC440 has two opcodes for icbt, add the missing one.
> > > 
> > > If you can do this when committing then please feel free to adjust this
> > > commit message as necessary, otherwise I'll change it in next iteration.
> > > 
> > > Regards,
> > > BALATON Zoltan
> > > 
> > > > A document reference to confim this would be nice.
> > > > 
> > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > ---
> > > > >  target/ppc/translate.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > > > index 5fe1ba6..3a215a1 100644
> > > > > --- a/target/ppc/translate.c
> > > > > +++ b/target/ppc/translate.c
> > > > > @@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
> > > > >  GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
> > > > >  GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
> > > > >                 PPC_BOOKE, PPC2_BOOKE206),
> > > > > +GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
> > > > > +               PPC_440_SPEC),
> > > > >  GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
> > > > >  GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
> > > > >  GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
> > > > 
> > > > 
> > > 
> > 
> > 
> 

-- 
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 v3 2/9] 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                | 13 ++++++++++++-
 include/hw/i2c/ppc4xx_i2c.h        |  4 ++++
 4 files changed, 18 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 4e0aaae..c0a1930 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,6 +47,11 @@
 
 #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)
+
 static void ppc4xx_i2c_reset(DeviceState *s)
 {
     PPC4xxI2CState *i2c = PPC4xx_I2C(s);
@@ -289,7 +295,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) {
@@ -322,6 +332,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");
+    s->bitbang = bitbang_i2c_init(s->bus);
 }
 
 static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
index e4b6ded..ea6c8e1 100644
--- a/include/hw/i2c/ppc4xx_i2c.h
+++ b/include/hw/i2c/ppc4xx_i2c.h
@@ -31,6 +31,9 @@
 #include "hw/sysbus.h"
 #include "hw/i2c/i2c.h"
 
+/* from hw/i2c/bitbang_i2c.h */
+typedef struct bitbang_i2c_interface bitbang_i2c_interface;
+
 #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
 #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
 
@@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
     I2CBus *bus;
     qemu_irq irq;
     MemoryRegion iomem;
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;
-- 
2.7.6

Re: [Qemu-devel] [PATCH v3 2/9] ppc4xx_i2c: Implement directcntl register

[David Gibson]

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

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Patch looks good, but it needs a commit message.  What is the
directcntl register?  Now does the bitbang interface play into that?
(Note that I know the answer to those questions right now, but it
needs to be in the commit message for the benefit of people looking
back in the future).

> ---
>  default-configs/ppc-softmmu.mak    |  1 +
>  default-configs/ppcemb-softmmu.mak |  1 +
>  hw/i2c/ppc4xx_i2c.c                | 13 ++++++++++++-
>  include/hw/i2c/ppc4xx_i2c.h        |  4 ++++
>  4 files changed, 18 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 4e0aaae..c0a1930 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,6 +47,11 @@
>  
>  #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)
> +
>  static void ppc4xx_i2c_reset(DeviceState *s)
>  {
>      PPC4xxI2CState *i2c = PPC4xx_I2C(s);
> @@ -289,7 +295,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) {
> @@ -322,6 +332,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");
> +    s->bitbang = bitbang_i2c_init(s->bus);
>  }
>  
>  static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index e4b6ded..ea6c8e1 100644
> --- a/include/hw/i2c/ppc4xx_i2c.h
> +++ b/include/hw/i2c/ppc4xx_i2c.h
> @@ -31,6 +31,9 @@
>  #include "hw/sysbus.h"
>  #include "hw/i2c/i2c.h"
>  
> +/* from hw/i2c/bitbang_i2c.h */
> +typedef struct bitbang_i2c_interface bitbang_i2c_interface;
> +
>  #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
>  #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
>  
> @@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
>      I2CBus *bus;
>      qemu_irq irq;
>      MemoryRegion iomem;
> +    bitbang_i2c_interface *bitbang;
>      uint8_t mdata;
>      uint8_t lmadr;
>      uint8_t hmadr;

-- 
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 v3 2/9] ppc4xx_i2c: Implement directcntl register

[BALATON Zoltan]

On Thu, 14 Jun 2018, David Gibson wrote:
> On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Patch looks good, but it needs a commit message.  What is the
> directcntl register?  Now does the bitbang interface play into that?
> (Note that I know the answer to those questions right now, but it
> needs to be in the commit message for the benefit of people looking
> back in the future).

If you know the answer could you please suggest an appropriate commit 
message? I'm a bit lost what should be written here that would explain the 
patch to someone who knows nothing about what this is. And for those who 
know, the patch itself should be fairly simple and not need more 
explanation.

Regards,
BALATON Zoltan

>> ---
>>  default-configs/ppc-softmmu.mak    |  1 +
>>  default-configs/ppcemb-softmmu.mak |  1 +
>>  hw/i2c/ppc4xx_i2c.c                | 13 ++++++++++++-
>>  include/hw/i2c/ppc4xx_i2c.h        |  4 ++++
>>  4 files changed, 18 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 4e0aaae..c0a1930 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,6 +47,11 @@
>>
>>  #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)
>> +
>>  static void ppc4xx_i2c_reset(DeviceState *s)
>>  {
>>      PPC4xxI2CState *i2c = PPC4xx_I2C(s);
>> @@ -289,7 +295,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) {
>> @@ -322,6 +332,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");
>> +    s->bitbang = bitbang_i2c_init(s->bus);
>>  }
>>
>>  static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
>> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
>> index e4b6ded..ea6c8e1 100644
>> --- a/include/hw/i2c/ppc4xx_i2c.h
>> +++ b/include/hw/i2c/ppc4xx_i2c.h
>> @@ -31,6 +31,9 @@
>>  #include "hw/sysbus.h"
>>  #include "hw/i2c/i2c.h"
>>
>> +/* from hw/i2c/bitbang_i2c.h */
>> +typedef struct bitbang_i2c_interface bitbang_i2c_interface;
>> +
>>  #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
>>  #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
>>
>> @@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
>>      I2CBus *bus;
>>      qemu_irq irq;
>>      MemoryRegion iomem;
>> +    bitbang_i2c_interface *bitbang;
>>      uint8_t mdata;
>>      uint8_t lmadr;
>>      uint8_t hmadr;
>
>

Re: [Qemu-devel] [PATCH v3 2/9] ppc4xx_i2c: Implement directcntl register

[David Gibson]

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

On Thu, Jun 14, 2018 at 09:51:33AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > 
> > Patch looks good, but it needs a commit message.  What is the
> > directcntl register?  Now does the bitbang interface play into that?
> > (Note that I know the answer to those questions right now, but it
> > needs to be in the commit message for the benefit of people looking
> > back in the future).
> 
> If you know the answer could you please suggest an appropriate commit
> message? I'm a bit lost what should be written here that would explain the
> patch to someone who knows nothing about what this is. And for those who
> know, the patch itself should be fairly simple and not need more
> explanation.

It's not really about explaining it to someone ho knows nothing about
what it is.  Rather it's about explaining to someone who has a general
familiarity but hasn't been looking at this code recently.  Try
imagining explaining itself to yourself in several years time having
been working on something entirely different.

So, perhaps:

| As well as being able to generate its own i2c transactions, the
| ppc4xx i2c controller has a DIRECTCNTL register which allows
| explicit control of the i2c lines.
|
| Using this register an OS can directly bitbang i2c operations.  In
| order to let emulated i2c devices respond to this, we need to wire
| up the DIRECTCNTL register to qemu's bitbanged i2c handling code.


> 
> Regards,
> BALATON Zoltan
> 
> > > ---
> > >  default-configs/ppc-softmmu.mak    |  1 +
> > >  default-configs/ppcemb-softmmu.mak |  1 +
> > >  hw/i2c/ppc4xx_i2c.c                | 13 ++++++++++++-
> > >  include/hw/i2c/ppc4xx_i2c.h        |  4 ++++
> > >  4 files changed, 18 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 4e0aaae..c0a1930 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,6 +47,11 @@
> > > 
> > >  #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)
> > > +
> > >  static void ppc4xx_i2c_reset(DeviceState *s)
> > >  {
> > >      PPC4xxI2CState *i2c = PPC4xx_I2C(s);
> > > @@ -289,7 +295,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) {
> > > @@ -322,6 +332,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");
> > > +    s->bitbang = bitbang_i2c_init(s->bus);
> > >  }
> > > 
> > >  static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
> > > diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> > > index e4b6ded..ea6c8e1 100644
> > > --- a/include/hw/i2c/ppc4xx_i2c.h
> > > +++ b/include/hw/i2c/ppc4xx_i2c.h
> > > @@ -31,6 +31,9 @@
> > >  #include "hw/sysbus.h"
> > >  #include "hw/i2c/i2c.h"
> > > 
> > > +/* from hw/i2c/bitbang_i2c.h */
> > > +typedef struct bitbang_i2c_interface bitbang_i2c_interface;
> > > +
> > >  #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
> > >  #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
> > > 
> > > @@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
> > >      I2CBus *bus;
> > >      qemu_irq irq;
> > >      MemoryRegion iomem;
> > > +    bitbang_i2c_interface *bitbang;
> > >      uint8_t mdata;
> > >      uint8_t lmadr;
> > >      uint8_t hmadr;
> > 
> > 
> 

-- 
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 v3 1/9] ppc4xx_i2c: Remove unimplemented sdata and intr registers

[BALATON Zoltan]

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

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/i2c/ppc4xx_i2c.c         | 16 +---------------
 include/hw/i2c/ppc4xx_i2c.h |  4 +---
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index d1936db..4e0aaae 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2007 Jocelyn Mayer
  * Copyright (c) 2012 François Revol
- * Copyright (c) 2016 BALATON Zoltan
+ * Copyright (c) 2016-2018 BALATON Zoltan
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -63,7 +63,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->mdcntl = 0;
     i2c->sts = 0;
     i2c->extsts = 0x8f;
-    i2c->sdata = 0;
     i2c->lsadr = 0;
     i2c->hsadr = 0;
     i2c->clkdiv = 0;
@@ -71,7 +70,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->xfrcnt = 0;
     i2c->xtcntlss = 0;
     i2c->directcntl = 0xf;
-    i2c->intr = 0;
 }
 
 static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
@@ -139,9 +137,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
                           TYPE_PPC4xx_I2C, __func__);
         }
         break;
-    case 2:
-        ret = i2c->sdata;
-        break;
     case 4:
         ret = i2c->lmadr;
         break;
@@ -181,9 +176,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
     case 16:
         ret = i2c->directcntl;
         break;
-    case 17:
-        ret = i2c->intr;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
@@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
             }
         }
         break;
-    case 2:
-        i2c->sdata = value;
-        break;
     case 4:
         i2c->lmadr = value;
         if (i2c_bus_busy(i2c->bus)) {
@@ -302,9 +291,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
     case 16:
         i2c->directcntl = value & 0x7;
         break;
-    case 17:
-        i2c->intr = value;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
index 3c60307..e4b6ded 100644
--- a/include/hw/i2c/ppc4xx_i2c.h
+++ b/include/hw/i2c/ppc4xx_i2c.h
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2007 Jocelyn Mayer
  * Copyright (c) 2012 François Revol
- * Copyright (c) 2016 BALATON Zoltan
+ * Copyright (c) 2016-2018 BALATON Zoltan
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
     uint8_t mdcntl;
     uint8_t sts;
     uint8_t extsts;
-    uint8_t sdata;
     uint8_t lsadr;
     uint8_t hsadr;
     uint8_t clkdiv;
@@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
     uint8_t xfrcnt;
     uint8_t xtcntlss;
     uint8_t directcntl;
-    uint8_t intr;
 } PPC4xxI2CState;
 
 #endif /* PPC4XX_I2C_H */
-- 
2.7.6

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

[David Gibson]

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

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

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

I'm guessing that's not what they're supposed to do, which is why
you're removing them, but that needs to be explained in the commit
message.

As a general rule cases where a one line commit message is acceptable
are *very* rare.

> ---
>  hw/i2c/ppc4xx_i2c.c         | 16 +---------------
>  include/hw/i2c/ppc4xx_i2c.h |  4 +---
>  2 files changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index d1936db..4e0aaae 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -3,7 +3,7 @@
>   *
>   * Copyright (c) 2007 Jocelyn Mayer
>   * Copyright (c) 2012 François Revol
> - * Copyright (c) 2016 BALATON Zoltan
> + * Copyright (c) 2016-2018 BALATON Zoltan
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
> @@ -63,7 +63,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>      i2c->mdcntl = 0;
>      i2c->sts = 0;
>      i2c->extsts = 0x8f;
> -    i2c->sdata = 0;
>      i2c->lsadr = 0;
>      i2c->hsadr = 0;
>      i2c->clkdiv = 0;
> @@ -71,7 +70,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>      i2c->xfrcnt = 0;
>      i2c->xtcntlss = 0;
>      i2c->directcntl = 0xf;
> -    i2c->intr = 0;
>  }
>  
>  static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
> @@ -139,9 +137,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>                            TYPE_PPC4xx_I2C, __func__);
>          }
>          break;
> -    case 2:
> -        ret = i2c->sdata;
> -        break;
>      case 4:
>          ret = i2c->lmadr;
>          break;
> @@ -181,9 +176,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>      case 16:
>          ret = i2c->directcntl;
>          break;
> -    case 17:
> -        ret = i2c->intr;
> -        break;
>      default:
>          if (addr < PPC4xx_I2C_MEM_SIZE) {
>              qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> @@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>              }
>          }
>          break;
> -    case 2:
> -        i2c->sdata = value;
> -        break;
>      case 4:
>          i2c->lmadr = value;
>          if (i2c_bus_busy(i2c->bus)) {
> @@ -302,9 +291,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>      case 16:
>          i2c->directcntl = value & 0x7;
>          break;
> -    case 17:
> -        i2c->intr = value;
> -        break;
>      default:
>          if (addr < PPC4xx_I2C_MEM_SIZE) {
>              qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index 3c60307..e4b6ded 100644
> --- a/include/hw/i2c/ppc4xx_i2c.h
> +++ b/include/hw/i2c/ppc4xx_i2c.h
> @@ -3,7 +3,7 @@
>   *
>   * Copyright (c) 2007 Jocelyn Mayer
>   * Copyright (c) 2012 François Revol
> - * Copyright (c) 2016 BALATON Zoltan
> + * Copyright (c) 2016-2018 BALATON Zoltan
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
> @@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
>      uint8_t mdcntl;
>      uint8_t sts;
>      uint8_t extsts;
> -    uint8_t sdata;
>      uint8_t lsadr;
>      uint8_t hsadr;
>      uint8_t clkdiv;
> @@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
>      uint8_t xfrcnt;
>      uint8_t xtcntlss;
>      uint8_t directcntl;
> -    uint8_t intr;
>  } PPC4xxI2CState;
>  
>  #endif /* PPC4XX_I2C_H */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[BALATON Zoltan]

On Thu, 14 Jun 2018, David Gibson wrote:
> On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> But.. they are implemented.  Albeit as an entirely software controlled
> register.

They were implemented only in that they could be written and read but did 
not implement any of the functionality they have in real hardware so in 
that sense they were not really implemented.

> I'm guessing that's not what they're supposed to do, which is why
> you're removing them, but that needs to be explained in the commit
> message.

Maybe adding this explanation:

We dont emulate slave mode so related registers are not needed. [lh]sadr 
are only retained to avoid too many warnings and simplify debugging but 
sdata is not even correct because device has a 4 byte FIFO instead so 
just remove this unimplemented register for now.

The intr register is also not implemented correctly, it is for diagnostics 
and normally not even visible on device without explicitly enabling it. As 
no guests are known to need this remove it as well.

Regards,
BALATON Zoltan

> As a general rule cases where a one line commit message is acceptable
> are *very* rare.
>
>> ---
>>  hw/i2c/ppc4xx_i2c.c         | 16 +---------------
>>  include/hw/i2c/ppc4xx_i2c.h |  4 +---
>>  2 files changed, 2 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>> index d1936db..4e0aaae 100644
>> --- a/hw/i2c/ppc4xx_i2c.c
>> +++ b/hw/i2c/ppc4xx_i2c.c
>> @@ -3,7 +3,7 @@
>>   *
>>   * Copyright (c) 2007 Jocelyn Mayer
>>   * Copyright (c) 2012 François Revol
>> - * Copyright (c) 2016 BALATON Zoltan
>> + * Copyright (c) 2016-2018 BALATON Zoltan
>>   *
>>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>>   * of this software and associated documentation files (the "Software"), to deal
>> @@ -63,7 +63,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>>      i2c->mdcntl = 0;
>>      i2c->sts = 0;
>>      i2c->extsts = 0x8f;
>> -    i2c->sdata = 0;
>>      i2c->lsadr = 0;
>>      i2c->hsadr = 0;
>>      i2c->clkdiv = 0;
>> @@ -71,7 +70,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>>      i2c->xfrcnt = 0;
>>      i2c->xtcntlss = 0;
>>      i2c->directcntl = 0xf;
>> -    i2c->intr = 0;
>>  }
>>
>>  static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
>> @@ -139,9 +137,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>>                            TYPE_PPC4xx_I2C, __func__);
>>          }
>>          break;
>> -    case 2:
>> -        ret = i2c->sdata;
>> -        break;
>>      case 4:
>>          ret = i2c->lmadr;
>>          break;
>> @@ -181,9 +176,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>>      case 16:
>>          ret = i2c->directcntl;
>>          break;
>> -    case 17:
>> -        ret = i2c->intr;
>> -        break;
>>      default:
>>          if (addr < PPC4xx_I2C_MEM_SIZE) {
>>              qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
>> @@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>              }
>>          }
>>          break;
>> -    case 2:
>> -        i2c->sdata = value;
>> -        break;
>>      case 4:
>>          i2c->lmadr = value;
>>          if (i2c_bus_busy(i2c->bus)) {
>> @@ -302,9 +291,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>      case 16:
>>          i2c->directcntl = value & 0x7;
>>          break;
>> -    case 17:
>> -        i2c->intr = value;
>> -        break;
>>      default:
>>          if (addr < PPC4xx_I2C_MEM_SIZE) {
>>              qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
>> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
>> index 3c60307..e4b6ded 100644
>> --- a/include/hw/i2c/ppc4xx_i2c.h
>> +++ b/include/hw/i2c/ppc4xx_i2c.h
>> @@ -3,7 +3,7 @@
>>   *
>>   * Copyright (c) 2007 Jocelyn Mayer
>>   * Copyright (c) 2012 François Revol
>> - * Copyright (c) 2016 BALATON Zoltan
>> + * Copyright (c) 2016-2018 BALATON Zoltan
>>   *
>>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>>   * of this software and associated documentation files (the "Software"), to deal
>> @@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
>>      uint8_t mdcntl;
>>      uint8_t sts;
>>      uint8_t extsts;
>> -    uint8_t sdata;
>>      uint8_t lsadr;
>>      uint8_t hsadr;
>>      uint8_t clkdiv;
>> @@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
>>      uint8_t xfrcnt;
>>      uint8_t xtcntlss;
>>      uint8_t directcntl;
>> -    uint8_t intr;
>>  } PPC4xxI2CState;
>>
>>  #endif /* PPC4XX_I2C_H */
>
>

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

[David Gibson]

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

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

That sounds reasonable.

> 
> Regards,
> BALATON Zoltan
> 
> > As a general rule cases where a one line commit message is acceptable
> > are *very* rare.
> > 
> > > ---
> > >  hw/i2c/ppc4xx_i2c.c         | 16 +---------------
> > >  include/hw/i2c/ppc4xx_i2c.h |  4 +---
> > >  2 files changed, 2 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > index d1936db..4e0aaae 100644
> > > --- a/hw/i2c/ppc4xx_i2c.c
> > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > @@ -3,7 +3,7 @@
> > >   *
> > >   * Copyright (c) 2007 Jocelyn Mayer
> > >   * Copyright (c) 2012 François Revol
> > > - * Copyright (c) 2016 BALATON Zoltan
> > > + * Copyright (c) 2016-2018 BALATON Zoltan
> > >   *
> > >   * Permission is hereby granted, free of charge, to any person obtaining a copy
> > >   * of this software and associated documentation files (the "Software"), to deal
> > > @@ -63,7 +63,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
> > >      i2c->mdcntl = 0;
> > >      i2c->sts = 0;
> > >      i2c->extsts = 0x8f;
> > > -    i2c->sdata = 0;
> > >      i2c->lsadr = 0;
> > >      i2c->hsadr = 0;
> > >      i2c->clkdiv = 0;
> > > @@ -71,7 +70,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
> > >      i2c->xfrcnt = 0;
> > >      i2c->xtcntlss = 0;
> > >      i2c->directcntl = 0xf;
> > > -    i2c->intr = 0;
> > >  }
> > > 
> > >  static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
> > > @@ -139,9 +137,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> > >                            TYPE_PPC4xx_I2C, __func__);
> > >          }
> > >          break;
> > > -    case 2:
> > > -        ret = i2c->sdata;
> > > -        break;
> > >      case 4:
> > >          ret = i2c->lmadr;
> > >          break;
> > > @@ -181,9 +176,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> > >      case 16:
> > >          ret = i2c->directcntl;
> > >          break;
> > > -    case 17:
> > > -        ret = i2c->intr;
> > > -        break;
> > >      default:
> > >          if (addr < PPC4xx_I2C_MEM_SIZE) {
> > >              qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> > > @@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > >              }
> > >          }
> > >          break;
> > > -    case 2:
> > > -        i2c->sdata = value;
> > > -        break;
> > >      case 4:
> > >          i2c->lmadr = value;
> > >          if (i2c_bus_busy(i2c->bus)) {
> > > @@ -302,9 +291,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > >      case 16:
> > >          i2c->directcntl = value & 0x7;
> > >          break;
> > > -    case 17:
> > > -        i2c->intr = value;
> > > -        break;
> > >      default:
> > >          if (addr < PPC4xx_I2C_MEM_SIZE) {
> > >              qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> > > diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> > > index 3c60307..e4b6ded 100644
> > > --- a/include/hw/i2c/ppc4xx_i2c.h
> > > +++ b/include/hw/i2c/ppc4xx_i2c.h
> > > @@ -3,7 +3,7 @@
> > >   *
> > >   * Copyright (c) 2007 Jocelyn Mayer
> > >   * Copyright (c) 2012 François Revol
> > > - * Copyright (c) 2016 BALATON Zoltan
> > > + * Copyright (c) 2016-2018 BALATON Zoltan
> > >   *
> > >   * Permission is hereby granted, free of charge, to any person obtaining a copy
> > >   * of this software and associated documentation files (the "Software"), to deal
> > > @@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
> > >      uint8_t mdcntl;
> > >      uint8_t sts;
> > >      uint8_t extsts;
> > > -    uint8_t sdata;
> > >      uint8_t lsadr;
> > >      uint8_t hsadr;
> > >      uint8_t clkdiv;
> > > @@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
> > >      uint8_t xfrcnt;
> > >      uint8_t xtcntlss;
> > >      uint8_t directcntl;
> > > -    uint8_t intr;
> > >  } PPC4xxI2CState;
> > > 
> > >  #endif /* PPC4XX_I2C_H */
> > 
> > 


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH v3 3/9] 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>
---
 hw/i2c/ppc4xx_i2c.c         | 222 +++++++++++++++++++++-----------------------
 include/hw/i2c/ppc4xx_i2c.h |   3 +-
 2 files changed, 110 insertions(+), 115 deletions(-)

diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index c0a1930..73d6ba5 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)
@@ -56,21 +69,13 @@ static void ppc4xx_i2c_reset(DeviceState *s)
 {
     PPC4xxI2CState *i2c = PPC4xx_I2C(s);
 
-    /* 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;
@@ -78,69 +83,29 @@ 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 *i2c = PPC4xx_I2C(opaque);
     uint64_t ret;
+    int i;
 
     switch (addr) {
     case 0:
-        ret = i2c->mdata;
-        if (ppc4xx_i2c_is_master(i2c)) {
+        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(i2c->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(i2c->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)) {
-                        /* 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:
@@ -160,6 +125,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
         break;
     case 9:
         ret = i2c->extsts;
+        ret |= !!i2c_bus_busy(i2c->bus) << 4;
         break;
     case 10:
         ret = i2c->lsadr;
@@ -203,70 +169,98 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
 
     switch (addr) {
     case 0:
-        i2c->mdata = value;
-        if (!i2c_bus_busy(i2c->bus)) {
-            /* assume we start a write transfer */
-            if (i2c_start_transfer(i2c->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(i2c->bus)) {
-            if (i2c_send(i2c->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->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(i2c->bus)) {
-            i2c_end_transfer(i2c->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(i2c->bus)) {
-                    /* end previous transfer */
-                    i2c->sts &= ~IIC_STS_PT;
-                    i2c_end_transfer(i2c->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(i2c->bus)) {
+            i2c_end_transfer(i2c->bus);
+            if (i2c->mdcntl & IIC_MDCNTL_EINT &&
+                i2c->intrmsk & IIC_INTRMSK_EIHE) {
+                    i2c->sts |= IIC_STS_IRQA;
+                    qemu_irq_raise(i2c->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(i2c->bus)) {
+                    i2c->extsts = (1 << 6);
+                    if (i2c_start_transfer(i2c->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(i2c->bus, &i2c->mdata[i], !recv)) {
+                        i2c->sts |= IIC_STS_ERR;
+                        i2c->extsts |= IIC_EXTSTS_XFRA;
+                        break;
                 }
-                if (i2c_start_transfer(i2c->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(i2c->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(i2c->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);
+        i2c->sts &= ~(value & 0x0a);
+        if (value & IIC_STS_IRQA && i2c->mdcntl & IIC_MDCNTL_EINT) {
+            qemu_irq_lower(i2c->irq);
+        }
         break;
     case 9:
         i2c->extsts &= ~(value & 0x8f);
@@ -287,12 +281,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(i2c));
             break;
         }
-        i2c->xtcntlss = value;
         break;
     case 16:
         i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
index ea6c8e1..0891a9c 100644
--- a/include/hw/i2c/ppc4xx_i2c.h
+++ b/include/hw/i2c/ppc4xx_i2c.h
@@ -46,7 +46,8 @@ typedef struct PPC4xxI2CState {
     qemu_irq irq;
     MemoryRegion iomem;
     bitbang_i2c_interface *bitbang;
-    uint8_t mdata;
+    int mdidx;
+    uint8_t mdata[4];
     uint8_t lmadr;
     uint8_t hmadr;
     uint8_t cntl;
-- 
2.7.6

[Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change

[BALATON Zoltan]

From: Sebastian Bauer <mail@sebastianbauer.info>

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 0625cf5..a2ee6e3 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -479,6 +479,7 @@ typedef struct SM501State {
     MemoryRegion twoD_engine_region;
     uint32_t last_width;
     uint32_t last_height;
+    uint32_t do_full_update; /* perform a full update next time */
     I2CBus *i2c_bus;
 
     /* mmio registers */
@@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
 
     assert(range_covers_byte(0, 0x400 * 3, addr));
     *(uint32_t *)&s->dc_palette[addr] = value;
+    s->do_full_update = 1;
 }
 
 static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
@@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
         full_update = 1;
     }
 
+    /* someone else requested a full update */
+    if (s->do_full_update) {
+        s->do_full_update = 0;
+        full_update = 1;
+    }
+
     /* draw each line according to conditions */
     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
-- 
2.7.6

Re: [Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change

[David Gibson]

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

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> From: Sebastian Bauer <mail@sebastianbauer.info>
> 
> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Commit message.  Why is this necessary?

> ---
>  hw/display/sm501.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 0625cf5..a2ee6e3 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -479,6 +479,7 @@ typedef struct SM501State {
>      MemoryRegion twoD_engine_region;
>      uint32_t last_width;
>      uint32_t last_height;
> +    uint32_t do_full_update; /* perform a full update next time */
>      I2CBus *i2c_bus;
>  
>      /* mmio registers */
> @@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
>  
>      assert(range_covers_byte(0, 0x400 * 3, addr));
>      *(uint32_t *)&s->dc_palette[addr] = value;
> +    s->do_full_update = 1;
>  }
>  
>  static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
> @@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
>          full_update = 1;
>      }
>  
> +    /* someone else requested a full update */
> +    if (s->do_full_update) {
> +        s->do_full_update = 0;
> +        full_update = 1;
> +    }
> +
>      /* draw each line according to conditions */
>      snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
>                offset, width * height * src_bpp, DIRTY_MEMORY_VGA);

-- 
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 v3 8/9] sm501: Perform a full update after palette change

[BALATON Zoltan]

On Thu, 14 Jun 2018, David Gibson wrote:
> On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
>> From: Sebastian Bauer <mail@sebastianbauer.info>
>>
>> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Commit message.  Why is this necessary?

Maybe because changing palette does not change display unless an update is 
done and dirty tracking which is used in update_display does not detect 
changes in device registers where palette is stored.

I'm not sure this is needed in all modes but I guess palette is not used 
in modes that are not indexed so unecessary updates should not happen evem 
in those cases because if palette is not used anyway, guest is unlikely to 
change it unless doing something really strange but we haven't seen any 
guests yet that would do that. Therefore this simple way of handling this 
should be OK.

Regards,
BALATON Zoltan

>> ---
>>  hw/display/sm501.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index 0625cf5..a2ee6e3 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -479,6 +479,7 @@ typedef struct SM501State {
>>      MemoryRegion twoD_engine_region;
>>      uint32_t last_width;
>>      uint32_t last_height;
>> +    uint32_t do_full_update; /* perform a full update next time */
>>      I2CBus *i2c_bus;
>>
>>      /* mmio registers */
>> @@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
>>
>>      assert(range_covers_byte(0, 0x400 * 3, addr));
>>      *(uint32_t *)&s->dc_palette[addr] = value;
>> +    s->do_full_update = 1;
>>  }
>>
>>  static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
>> @@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
>>          full_update = 1;
>>      }
>>
>> +    /* someone else requested a full update */
>> +    if (s->do_full_update) {
>> +        s->do_full_update = 0;
>> +        full_update = 1;
>> +    }
>> +
>>      /* draw each line according to conditions */
>>      snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
>>                offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
>
>

Re: [Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change

[David Gibson]

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

On Thu, Jun 14, 2018 at 10:00:50AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> > > From: Sebastian Bauer <mail@sebastianbauer.info>
> > > 
> > > Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > 
> > Commit message.  Why is this necessary?
> 
> Maybe because changing palette does not change display unless an update is
> done and dirty tracking which is used in update_display does not detect
> changes in device registers where palette is stored.
> 
> I'm not sure this is needed in all modes but I guess palette is not used in
> modes that are not indexed so unecessary updates should not happen evem in
> those cases because if palette is not used anyway, guest is unlikely to
> change it unless doing something really strange but we haven't seen any
> guests yet that would do that. Therefore this simple way of handling this
> should be OK.

Sure, so, put that into a commit message in the next spin.

> 
> Regards,
> BALATON Zoltan
> 
> > > ---
> > >  hw/display/sm501.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > > index 0625cf5..a2ee6e3 100644
> > > --- a/hw/display/sm501.c
> > > +++ b/hw/display/sm501.c
> > > @@ -479,6 +479,7 @@ typedef struct SM501State {
> > >      MemoryRegion twoD_engine_region;
> > >      uint32_t last_width;
> > >      uint32_t last_height;
> > > +    uint32_t do_full_update; /* perform a full update next time */
> > >      I2CBus *i2c_bus;
> > > 
> > >      /* mmio registers */
> > > @@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
> > > 
> > >      assert(range_covers_byte(0, 0x400 * 3, addr));
> > >      *(uint32_t *)&s->dc_palette[addr] = value;
> > > +    s->do_full_update = 1;
> > >  }
> > > 
> > >  static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
> > > @@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
> > >          full_update = 1;
> > >      }
> > > 
> > > +    /* someone else requested a full update */
> > > +    if (s->do_full_update) {
> > > +        s->do_full_update = 0;
> > > +        full_update = 1;
> > > +    }
> > > +
> > >      /* draw each line according to conditions */
> > >      snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
> > >                offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
> > 
> > 
> 

-- 
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 v3 5/9] 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

Re: [Qemu-devel] [PATCH v3 5/9] sam460ex: Add RTC device

[David Gibson]

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

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> The Sam460ex has an M41T80 serial RTC chip on I2C bus 0 at address 0x68.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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

-- 
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 v3 6/9] sm501: Do not clear read only bits when writing registers

[BALATON Zoltan]

When writing registers that have read only bits we have to avoid
changing these bits as they may have non zero values. Make sure we use
the correct masks to mask out read only and reserved bits when
changing registers.

Also remove extra spaces from dram_control and arbitration_control
assignments.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v3: Not only preserve read only bits but also allow clearing r/w bits

 hw/display/sm501.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index e47be99..ca0840f 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -836,27 +836,30 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
 
     switch (addr) {
     case SM501_SYSTEM_CONTROL:
-        s->system_control = value & 0xE300B8F7;
+        s->system_control &= 0x10DB0000;
+        s->system_control |= value & 0xEF00B8F7;
         break;
     case SM501_MISC_CONTROL:
-        s->misc_control = value & 0xFF7FFF20;
+        s->misc_control &= 0xEF;
+        s->misc_control |= value & 0xFF7FFF10;
         break;
     case SM501_GPIO31_0_CONTROL:
         s->gpio_31_0_control = value;
         break;
     case SM501_GPIO63_32_CONTROL:
-        s->gpio_63_32_control = value;
+        s->gpio_63_32_control = value & 0xFF80FFFF;
         break;
     case SM501_DRAM_CONTROL:
         s->local_mem_size_index = (value >> 13) & 0x7;
         /* TODO : check validity of size change */
-        s->dram_control |=  value & 0x7FFFFFC3;
+        s->dram_control &= 0x80000000;
+        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;
+        s->irq_mask = value & 0xFFDF3F5F;
         break;
     case SM501_MISC_TIMING:
         s->misc_timing = value & 0xF31F1FFF;
-- 
2.7.6

Re: [Qemu-devel] [PATCH v3 6/9] sm501: Do not clear read only bits when writing registers

[David Gibson]

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

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> When writing registers that have read only bits we have to avoid
> changing these bits as they may have non zero values. Make sure we use
> the correct masks to mask out read only and reserved bits when
> changing registers.
> 
> Also remove extra spaces from dram_control and arbitration_control
> assignments.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> v3: Not only preserve read only bits but also allow clearing r/w
> bits

Applied to ppc-for-3.0, thanks.

> 
>  hw/display/sm501.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index e47be99..ca0840f 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -836,27 +836,30 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>  
>      switch (addr) {
>      case SM501_SYSTEM_CONTROL:
> -        s->system_control = value & 0xE300B8F7;
> +        s->system_control &= 0x10DB0000;
> +        s->system_control |= value & 0xEF00B8F7;
>          break;
>      case SM501_MISC_CONTROL:
> -        s->misc_control = value & 0xFF7FFF20;
> +        s->misc_control &= 0xEF;
> +        s->misc_control |= value & 0xFF7FFF10;
>          break;
>      case SM501_GPIO31_0_CONTROL:
>          s->gpio_31_0_control = value;
>          break;
>      case SM501_GPIO63_32_CONTROL:
> -        s->gpio_63_32_control = value;
> +        s->gpio_63_32_control = value & 0xFF80FFFF;
>          break;
>      case SM501_DRAM_CONTROL:
>          s->local_mem_size_index = (value >> 13) & 0x7;
>          /* TODO : check validity of size change */
> -        s->dram_control |=  value & 0x7FFFFFC3;
> +        s->dram_control &= 0x80000000;
> +        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;
> +        s->irq_mask = value & 0xFFDF3F5F;
>          break;
>      case SM501_MISC_TIMING:
>          s->misc_timing = value & 0xF31F1FFF;

-- 
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 v3 4/9] 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>
---

Notes:
    v3: Fixed \n-s in log messages

 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 8a94517..74ae589 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -829,6 +829,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..734d7d9
--- /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, "%s: unimplemented register: %d\n",
+                      __func__, s->addr - 1);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid register: %d\n",
+                      __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 v3 4/9] hw/timer: Add basic M41T80 emulation

[David Gibson]

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

On Thu, Jun 14, 2018 at 02:17:00AM +0200, 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>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> 
> Notes:
>     v3: Fixed \n-s in log messages
> 
>  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 8a94517..74ae589 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -829,6 +829,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..734d7d9
> --- /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, "%s: unimplemented register: %d\n",
> +                      __func__, s->addr - 1);
> +        return 0;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid register: %d\n",
> +                      __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)

-- 
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 v3 7/9] 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 ca0840f..0625cf5 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;
@@ -897,6 +907,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;
@@ -1577,6 +1688,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;
@@ -1615,6 +1730,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),
@@ -1622,6 +1742,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);
@@ -1705,6 +1828,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()
      }
 };
@@ -1770,8 +1898,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),
@@ -1843,8 +1971,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

Re: [Qemu-devel] [PATCH v3 7/9] sm501: Implement i2c part for reading monitor EDID

[David Gibson]

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

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Again needs a commit message expanding on what this is and why it's
useful.

[snip]
>  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),

Additionally, since you're changing the migration stream, you need to
point out that this is not used in any machine types which support
cross-version migration.

> @@ -1843,8 +1971,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,

-- 
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 v3 7/9] sm501: Implement i2c part for reading monitor EDID

[BALATON Zoltan]

On Thu, 14 Jun 2018, David Gibson wrote:
> On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Again needs a commit message expanding on what this is and why it's
> useful.

What else need to be explained apart from that SM501 has an i2c part which 
is used to access EDID info of attached monitor. But this is what the 
commit title already says. This patch implements that so far not emulated 
part of the chip.

> [snip]
>>  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),
>
> Additionally, since you're changing the migration stream, you need to
> point out that this is not used in any machine types which support
> cross-version migration.

AFAIK it is only used in SH4 and PPC sam460ex.

Re: [Qemu-devel] [PATCH v3 7/9] sm501: Implement i2c part for reading monitor EDID

[David Gibson]

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

On Thu, Jun 14, 2018 at 10:06:33AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > 
> > Again needs a commit message expanding on what this is and why it's
> > useful.
> 
> What else need to be explained apart from that SM501 has an i2c part which
> is used to access EDID info of attached monitor. But this is what the commit
> title already says. This patch implements that so far not emulated part of
> the chip.

In this case it would be sufficient just to say the same thing, but in
a less compressed form than the subject line.  e.g.

| The sm501 controller includes an i2c slave from which the monitor's
| EDID information can be read.  This wasn't yet implemented in qemu,
| so this patch adds it.
|
| (The sm501 part is used only by the SH4 and PPC sam460ex machine
| types in qemu)

> 
> > [snip]
> > >  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),
> > 
> > Additionally, since you're changing the migration stream, you need to
> > point out that this is not used in any machine types which support
> > cross-version migration.
> 
> AFAIK it is only used in SH4 and PPC sam460ex.
> 

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