[Qemu-devel] [PATCH v3 0/6] aspeed: add a witherspoon-bmc machine

[Qemu-devel] [PATCH v3 0/6] aspeed: add a witherspoon-bmc machine

[Cédric Le Goater]

Hello,

This series adds a new Aspeed machine to emulate the BMC of a
Witherspoon system. It also extends the other Aspeed machines with I2C
devices and adds a simple model for the pca9552 LED blinker present on
the witherspoon board.

Thanks,

C.

Changes since v2:

 - removed comments on the I2C buffer size pf the PCA9552 device.
 - removed 'ignore_memory_transaction_failures' flag on the
   witherspoon machine.
   
Changes since v1:

 - introduced smbus_eeprom_init_one()

Cédric Le Goater (6):
  aspeed: add support for the witherspoon-bmc board
  aspeed: add an I2C RTC device to all machines
  smbus: add a smbus_eeprom_init_one() routine
  aspeed: Add EEPROM I2C devices
  misc: add pca9552 LED blinker model
  aspeed: add the pc9552 chips to the witherspoon machine

 default-configs/arm-softmmu.mak |   1 +
 hw/arm/aspeed.c                 |  85 ++++++++++++++++
 hw/i2c/smbus_eeprom.c           |  16 ++-
 hw/misc/Makefile.objs           |   1 +
 hw/misc/pca9552.c               | 212 ++++++++++++++++++++++++++++++++++++++++
 include/hw/i2c/smbus.h          |   1 +
 include/hw/misc/pca9552.h       |  32 ++++++
 7 files changed, 343 insertions(+), 5 deletions(-)
 create mode 100644 hw/misc/pca9552.c
 create mode 100644 include/hw/misc/pca9552.h

-- 
2.13.6

[Qemu-devel] [PATCH v3 1/6] aspeed: add support for the witherspoon-bmc board

[Cédric Le Goater]

The Witherspoon boards are OpenPOWER system hosting POWER9 Processors.
Let's add support for their BMC including a couple of I2C devices as
found on real HW.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---

 Changes since v2:

 - removed 'ignore_memory_transaction_failures' flag on the machine.
   Will require a couple of fixes in the legacy U-Boot accessing wrongly
   the bss before the DRAM is initialized.

   The HW drops the write access surely because the SMC controller is
   configured in autoread mode by default. I don't know how to model
   this behavior in QEMU. I think we would need suppport to boot from
   a MMIO region.

 - Added a comment on the I2C device used.

 hw/arm/aspeed.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index ab895ad490af..5e0b9a184b9c 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -46,6 +46,7 @@ enum {
     PALMETTO_BMC,
     AST2500_EVB,
     ROMULUS_BMC,
+    WITHERSPOON_BMC,
 };
 
 /* Palmetto hardware value: 0x120CE416 */
@@ -83,8 +84,12 @@ enum {
         SCU_AST2500_HW_STRAP_ACPI_ENABLE |                              \
         SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_MASTER))
 
+/* Witherspoon hardware value: 0xF10AD216 (but use romulus definition) */
+#define WITHERSPOON_BMC_HW_STRAP1 ROMULUS_BMC_HW_STRAP1
+
 static void palmetto_bmc_i2c_init(AspeedBoardState *bmc);
 static void ast2500_evb_i2c_init(AspeedBoardState *bmc);
+static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc);
 
 static const AspeedBoardConfig aspeed_boards[] = {
     [PALMETTO_BMC] = {
@@ -110,6 +115,14 @@ static const AspeedBoardConfig aspeed_boards[] = {
         .spi_model = "mx66l1g45g",
         .num_cs    = 2,
     },
+    [WITHERSPOON_BMC]  = {
+        .soc_name  = "ast2500-a1",
+        .hw_strap1 = WITHERSPOON_BMC_HW_STRAP1,
+        .fmc_model = "mx25l25635e",
+        .spi_model = "mx66l1g45g",
+        .num_cs    = 2,
+        .i2c_init  = witherspoon_bmc_i2c_init,
+    },
 };
 
 #define FIRMWARE_ADDR 0x0
@@ -337,11 +350,47 @@ static const TypeInfo romulus_bmc_type = {
     .class_init = romulus_bmc_class_init,
 };
 
+static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
+{
+    AspeedSoCState *soc = &bmc->soc;
+
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
+
+    /* The Witherspoon expects a TMP275 but a TMP105 is compatible */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), "tmp105", 0x4a);
+}
+
+static void witherspoon_bmc_init(MachineState *machine)
+{
+    aspeed_board_init(machine, &aspeed_boards[WITHERSPOON_BMC]);
+}
+
+static void witherspoon_bmc_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->desc = "OpenPOWER Witherspoon BMC (ARM1176)";
+    mc->init = witherspoon_bmc_init;
+    mc->max_cpus = 1;
+    mc->no_sdcard = 1;
+    mc->no_floppy = 1;
+    mc->no_cdrom = 1;
+    mc->no_parallel = 1;
+}
+
+static const TypeInfo witherspoon_bmc_type = {
+    .name = MACHINE_TYPE_NAME("witherspoon-bmc"),
+    .parent = TYPE_MACHINE,
+    .class_init = witherspoon_bmc_class_init,
+};
+
 static void aspeed_machine_init(void)
 {
     type_register_static(&palmetto_bmc_type);
     type_register_static(&ast2500_evb_type);
     type_register_static(&romulus_bmc_type);
+    type_register_static(&witherspoon_bmc_type);
 }
 
 type_init(aspeed_machine_init)
-- 
2.13.6

Re: [Qemu-devel] [PATCH v3 1/6] aspeed: add support for the witherspoon-bmc board

[Peter Maydell]

On 13 October 2017 at 15:28, Cédric Le Goater <clg@kaod.org> wrote:
> The Witherspoon boards are OpenPOWER system hosting POWER9 Processors.
> Let's add support for their BMC including a couple of I2C devices as
> found on real HW.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>
>  Changes since v2:
>
>  - removed 'ignore_memory_transaction_failures' flag on the machine.
>    Will require a couple of fixes in the legacy U-Boot accessing wrongly
>    the bss before the DRAM is initialized.
>
>    The HW drops the write access surely because the SMC controller is
>    configured in autoread mode by default. I don't know how to model
>    this behavior in QEMU. I think we would need suppport to boot from
>    a MMIO region.

It is actually possible now -- see hw/ssi/xilinx_spips.c and the
request_mmio_ptr code. It's a bit experimental at the moment
though (in particular it breaks migration, a thing which we
seem to have forgotten to fix for this release cycle :-( )

The other similar thing we have is the flash devices, which
allow execution, but requires that the guest doesn't try to
execute at the same time as it's put the flash into
reads-like-a-device mode.

How are you modelling this bit of the address space at the moment?
A "reads like RAM but writes are ignored" lump of memory is easy...

In any case we can deal with this as a later patchset.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/6] aspeed: add support for the witherspoon-bmc board

[Cédric Le Goater]

On 10/17/2017 05:39 PM, Peter Maydell wrote:
> On 13 October 2017 at 15:28, Cédric Le Goater <clg@kaod.org> wrote:
>> The Witherspoon boards are OpenPOWER system hosting POWER9 Processors.
>> Let's add support for their BMC including a couple of I2C devices as
>> found on real HW.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>> ---
>>
>>  Changes since v2:
>>
>>  - removed 'ignore_memory_transaction_failures' flag on the machine.
>>    Will require a couple of fixes in the legacy U-Boot accessing wrongly
>>    the bss before the DRAM is initialized.
>>
>>    The HW drops the write access surely because the SMC controller is
>>    configured in autoread mode by default. I don't know how to model
>>    this behavior in QEMU. I think we would need suppport to boot from
>>    a MMIO region.
> 
> It is actually possible now -- see hw/ssi/xilinx_spips.c and the
> request_mmio_ptr code. It's a bit experimental at the moment
> though (in particular it breaks migration, a thing which we
> seem to have forgotten to fix for this release cycle :-( )

I see. So the Aspeed SMC model would have to maintain a cache area 
of the flash device contents to satisfy the request_ptr() call ?   

> The other similar thing we have is the flash devices, which
> allow execution, but requires that the guest doesn't try to
> execute at the same time as it's put the flash into
> reads-like-a-device mode.
> 
> How are you modelling this bit of the address space at the moment?

The different flash devices are mapped on the AHB bus in ranges 
depending on the controller (and the SoC). For the AST2500, which 
is the witherspoon SoC, we have :

  [2000:0000-2FFF:FFFF]  BMC SPI Flash Memory
  [3000:0000-37FF:FFFF]  SPI1 Flash Memory
  [3800:0000-3FFF:FFFF]  SPI2 Flash Memory

The SoC boots from CS0 of the BMC SPI Controller which is also 
mapped at :

  [0000:0000-0FFF:FFFF]  Firmware SPI Memory (boot-up default)

Today, we  have created a ROM region which is filled with the 
CS0 flash content. See commit d769a1da342d ("aspeed: use first 
FMC flash as a boot ROM")

So we end up with address-space memory like this :

    0000000000000000-ffffffffffffffff (prio 0, i/o): system
      0000000000000000-0000000007ffffff (prio 0, rom): aspeed.boot_rom
      ...
      0000000020000000-000000002fffffff (prio 0, i/o): aspeed.smc.ast2500-fmc.flash
        0000000020000000-0000000027ffffff (prio 0, i/o): aspeed.smc.ast2500-fmc.0
        0000000028000000-0000000029ffffff (prio 0, i/o): aspeed.smc.ast2500-fmc.1
        000000002a000000-000000002bffffff (prio 0, i/o): aspeed.smc.ast2500-fmc.2
      0000000030000000-0000000037ffffff (prio 0, i/o): aspeed.smc.ast2500-spi1.flash
      ...   
  
This is a bit brutal, but it boots from the flash device. Booting 
from the MMIO region would be much better. I will take a look at it, 
but after some PPC work. 

> A "reads like RAM but writes are ignored" lump of memory is easy...

yes. It seems but I haven't found my way through :/ a stack trace
of the failure looks like this :

#0  0x00005555558c1680 in arm_cpu_do_transaction_failed (cs=0x5555568cfc00, physaddr=537117152, addr=209376, size=4, access_type=MMU_DATA_STORE, mmu_idx=3, attrs=..., response=2, retaddr=140737054425367)
    at target/arm/op_helper.c:241
#1  0x00005555557e36c6 in cpu_transaction_failed (retaddr=140737054425367, response=2, attrs=..., mmu_idx=3, access_type=MMU_DATA_STORE, size=4, addr=209376, physaddr=537117152, cpu=0x5555568cfc00)
    at include/qom/cpu.h:895
#2  0x00005555557e36c6 in io_writex (env=env@entry=0x5555568d7e90, iotlbentry=<optimized out>, mmu_idx=mmu_idx@entry=3, val=val@entry=4294967, addr=addr@entry=209376, retaddr=retaddr@entry=140737054425367, size=4) at accel/tcg/cputlb.c:825
#3  0x00005555557e73ef in io_writel (retaddr=140737054425367, addr=209376, val=4294967, index=204, mmu_idx=3, env=0x5555568d7e90) at accel/tcg/softmmu_template.h:265
#4  0x00005555557e73ef in helper_le_stl_mmu (env=0x5555568d7e90, addr=<optimized out>, val=4294967, oi=35, retaddr=140737054425367) at accel/tcg/softmmu_template.h:300
#5  0x00007fffe622c117 in code_gen_buffer ()
#6  0x00005555557eef8c in cpu_tb_exec (itb=<optimized out>, itb=<optimized out>, cpu=0x5555568d7e90)


> In any case we can deal with this as a later patchset.

Well, I would prefer fixing that little issue above with a workaround
region. 

Thanks,

C. 

Re: [Qemu-devel] [PATCH v3 1/6] aspeed: add support for the witherspoon-bmc board

[Peter Maydell]

On 18 October 2017 at 17:10, Cédric Le Goater <clg@kaod.org> wrote:
> On 10/17/2017 05:39 PM, Peter Maydell wrote:
>> A "reads like RAM but writes are ignored" lump of memory is easy...
>
> yes. It seems but I haven't found my way through :/

memory_region_init_rom_device() creates a memory region
which is backed by RAM for reads (and for debug writes,
like ELF file loading), but backed by the usual
device callback functions for writes. So I think you should
just be able to use one of those but with the write
functions doing nothing (or warning about unimp. behaviour,
assuming that your flash devices actually have some
kind of command behaviour for writes).

This is how we implement the flash device with actual
behaviour behind them.

hw/mips/boston.c actually has an example of doing a dummy
flash device like this (though it uses
memory_region_init_rom_device_nomigrate(), which is a bug
because your memory contents won't get migrated if you
do that. Use the plain function instead.)

thanks
-- PMM

[Qemu-devel] [PATCH v3 2/6] aspeed: add an I2C RTC device to all machines

[Cédric Le Goater]

The AST2500 EVB does not have an RTC but we can pretend that one is
plugged on the I2C bus header.

The romulus and witherspoon boards expects an Epson RX8900 I2C RTC but
a ds1338 is good enough for the basic features we need.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/arm/aspeed.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 5e0b9a184b9c..032fd61d452f 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -89,6 +89,7 @@ enum {
 
 static void palmetto_bmc_i2c_init(AspeedBoardState *bmc);
 static void ast2500_evb_i2c_init(AspeedBoardState *bmc);
+static void romulus_bmc_i2c_init(AspeedBoardState *bmc);
 static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc);
 
 static const AspeedBoardConfig aspeed_boards[] = {
@@ -114,6 +115,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
         .fmc_model = "n25q256a",
         .spi_model = "mx66l1g45g",
         .num_cs    = 2,
+        .i2c_init  = romulus_bmc_i2c_init,
     },
     [WITHERSPOON_BMC]  = {
         .soc_name  = "ast2500-a1",
@@ -298,6 +300,10 @@ static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
 
     /* The AST2500 EVB expects a LM75 but a TMP105 is compatible */
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7), "tmp105", 0x4d);
+
+    /* The AST2500 EVB does not have an RTC. Let's pretend that one is
+     * plugged on the I2C bus header */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
 }
 
 static void ast2500_evb_init(MachineState *machine)
@@ -325,6 +331,15 @@ static const TypeInfo ast2500_evb_type = {
     .class_init = ast2500_evb_class_init,
 };
 
+static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
+{
+    AspeedSoCState *soc = &bmc->soc;
+
+    /* The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
+     * good enough */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
+}
+
 static void romulus_bmc_init(MachineState *machine)
 {
     aspeed_board_init(machine, &aspeed_boards[ROMULUS_BMC]);
@@ -359,6 +374,10 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
 
     /* The Witherspoon expects a TMP275 but a TMP105 is compatible */
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), "tmp105", 0x4a);
+
+    /* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
+     * good enough */
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
 }
 
 static void witherspoon_bmc_init(MachineState *machine)
-- 
2.13.6

[Qemu-devel] [PATCH v3 3/6] smbus: add a smbus_eeprom_init_one() routine

[Cédric Le Goater]

This is an helper routine to add a single EEPROM on an I2C bus. It can
be directly used by smbus_eeprom_init() which adds a certain number of
EEPROMs on mips and x86 machines.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i2c/smbus_eeprom.c  | 16 +++++++++++-----
 include/hw/i2c/smbus.h |  1 +
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index b13ec0fe7a2a..2d24a4cd59bf 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -140,6 +140,16 @@ static void smbus_eeprom_register_types(void)
 
 type_init(smbus_eeprom_register_types)
 
+void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
+{
+    DeviceState *dev;
+
+    dev = qdev_create((BusState *) smbus, "smbus-eeprom");
+    qdev_prop_set_uint8(dev, "address", address);
+    qdev_prop_set_ptr(dev, "data", eeprom_buf);
+    qdev_init_nofail(dev);
+}
+
 void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int eeprom_spd_size)
 {
@@ -150,10 +160,6 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
     }
 
     for (i = 0; i < nb_eeprom; i++) {
-        DeviceState *eeprom;
-        eeprom = qdev_create((BusState *)smbus, "smbus-eeprom");
-        qdev_prop_set_uint8(eeprom, "address", 0x50 + i);
-        qdev_prop_set_ptr(eeprom, "data", eeprom_buf + (i * 256));
-        qdev_init_nofail(eeprom);
+        smbus_eeprom_init_one(smbus, 0x50 + i, eeprom_buf + (i * 256));
     }
 }
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index 544bbc19574f..666cdeb04c07 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -77,6 +77,7 @@ int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data);
 int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
                       int len);
 
+void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
 void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int size);
 
-- 
2.13.6

[Qemu-devel] [PATCH v3 4/6] aspeed: Add EEPROM I2C devices

[Cédric Le Goater]

The Aspeed boards have at least one EEPROM to hold the Vital Product
Data (VPD).

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/arm/aspeed.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 032fd61d452f..2a6ca58527f8 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -17,6 +17,7 @@
 #include "hw/arm/arm.h"
 #include "hw/arm/aspeed_soc.h"
 #include "hw/boards.h"
+#include "hw/i2c/smbus.h"
 #include "qemu/log.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
@@ -255,11 +256,15 @@ static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
     DeviceState *dev;
+    uint8_t *eeprom_buf = g_malloc0(32 * 1024);
 
     /* The palmetto platform expects a ds3231 RTC but a ds1338 is
      * enough to provide basic RTC features. Alarms will be missing */
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), "ds1338", 0x68);
 
+    smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 0), 0x50,
+                          eeprom_buf);
+
     /* add a TMP423 temperature sensor */
     dev = i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 2),
                            "tmp423", 0x4c);
@@ -297,6 +302,10 @@ static const TypeInfo palmetto_bmc_type = {
 static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
+    uint8_t *eeprom_buf = g_malloc0(8 * 1024);
+
+    smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), 0x50,
+                          eeprom_buf);
 
     /* The AST2500 EVB expects a LM75 but a TMP105 is compatible */
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 7), "tmp105", 0x4d);
@@ -368,6 +377,7 @@ static const TypeInfo romulus_bmc_type = {
 static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
 {
     AspeedSoCState *soc = &bmc->soc;
+    uint8_t *eeprom_buf = g_malloc0(8 * 1024);
 
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
@@ -378,6 +388,9 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
     /* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
      * good enough */
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "ds1338", 0x32);
+
+    smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
+                          eeprom_buf);
 }
 
 static void witherspoon_bmc_init(MachineState *machine)
-- 
2.13.6

[Qemu-devel] [PATCH v3 5/6] misc: add pca9552 LED blinker model

[Cédric Le Goater]

Specs are available here :

  https://www.nxp.com/docs/en/data-sheet/PCA9552.pdf

This is a simple model supporting the basic registers for led and GPIO
mode. The device also supports two blinking rates but not the model
yet.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 Changes since v2:

 - removed comments on the I2C buffer size, but kept the array. I did
   not want to rewrite the buffer handling

 default-configs/arm-softmmu.mak |   1 +
 hw/misc/Makefile.objs           |   1 +
 hw/misc/pca9552.c               | 212 ++++++++++++++++++++++++++++++++++++++++
 include/hw/misc/pca9552.h       |  32 ++++++
 4 files changed, 246 insertions(+)
 create mode 100644 hw/misc/pca9552.c
 create mode 100644 include/hw/misc/pca9552.h

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 5059d134c816..d868d1095a6c 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -16,6 +16,7 @@ CONFIG_TSC2005=y
 CONFIG_LM832X=y
 CONFIG_TMP105=y
 CONFIG_TMP421=y
+CONFIG_PCA9552=y
 CONFIG_STELLARIS=y
 CONFIG_STELLARIS_INPUT=y
 CONFIG_STELLARIS_ENET=y
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index e8f0a02f35af..e4e22880dbbc 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -7,6 +7,7 @@ common-obj-$(CONFIG_SGA) += sga.o
 common-obj-$(CONFIG_ISA_TESTDEV) += pc-testdev.o
 common-obj-$(CONFIG_PCI_TESTDEV) += pci-testdev.o
 common-obj-$(CONFIG_EDU) += edu.o
+common-obj-$(CONFIG_PCA9552) += pca9552.o
 
 common-obj-y += unimp.o
 
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
new file mode 100644
index 000000000000..22460f4c14fe
--- /dev/null
+++ b/hw/misc/pca9552.c
@@ -0,0 +1,212 @@
+/*
+ * PCA9552 I2C LED blinker
+ *
+ * Copyright (c) 2017, IBM Corporation.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/hw.h"
+#include "hw/misc/pca9552.h"
+
+#define PCA9552_INPUT0   0 /* read only input register 0 */
+#define PCA9552_INPUT1   1 /* read only input register 1  */
+#define PCA9552_PSC0     2 /* read/write frequency prescaler 0 */
+#define PCA9552_PWM0     3 /* read/write PWM register 0 */
+#define PCA9552_PSC1     4 /* read/write frequency prescaler 1 */
+#define PCA9552_PWM1     5 /* read/write PWM register 1 */
+#define PCA9552_LS0      6 /* read/write LED0 to LED3 selector */
+#define PCA9552_LS1      7 /* read/write LED4 to LED7 selector */
+#define PCA9552_LS2      8 /* read/write LED8 to LED11 selector */
+#define PCA9552_LS3      9 /* read/write LED12 to LED15 selector */
+
+#define PCA9552_LED_ON   0x0
+#define PCA9552_LED_OFF  0x1
+#define PCA9552_LED_PWM0 0x2
+#define PCA9552_LED_PWM1 0x3
+
+static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
+{
+    uint8_t reg   = PCA9552_LS0 + (pin / 4);
+    uint8_t shift = (pin % 4) << 1;
+
+    return (s->regs[reg] >> shift) & 0x3;
+}
+
+static void pca9552_update_pin_input(PCA9552State *s)
+{
+    int i;
+
+    for (i = 0; i < 16; i++) {
+        uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
+        uint8_t input_shift = (i % 8);
+        uint8_t config = pca9552_pin_get_config(s, i);
+
+        switch (config) {
+        case PCA9552_LED_ON:
+            s->regs[input_reg] |= 1 << input_shift;
+            break;
+        case PCA9552_LED_OFF:
+            s->regs[input_reg] &= ~(1 << input_shift);
+            break;
+        case PCA9552_LED_PWM0:
+        case PCA9552_LED_PWM1:
+            /* ??? */
+        default:
+            break;
+        }
+    }
+}
+
+static void pca9552_read(PCA9552State *s)
+{
+    uint8_t reg = s->pointer & 0xf;
+
+    s->len = 0;
+
+    switch (reg) {
+    case PCA9552_INPUT0:
+    case PCA9552_INPUT1:
+    case PCA9552_PSC0:
+    case PCA9552_PWM0:
+    case PCA9552_PSC1:
+    case PCA9552_PWM1:
+    case PCA9552_LS0:
+    case PCA9552_LS1:
+    case PCA9552_LS2:
+    case PCA9552_LS3:
+        s->buf[s->len++] = s->regs[reg];
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to register %d\n",
+                      __func__, reg);
+    }
+}
+
+static void pca9552_write(PCA9552State *s)
+{
+    uint8_t reg = s->pointer & 0xf;
+
+    switch (reg) {
+    case PCA9552_PSC0:
+    case PCA9552_PWM0:
+    case PCA9552_PSC1:
+    case PCA9552_PWM1:
+        s->regs[reg] = s->buf[0];
+        break;
+
+    case PCA9552_LS0:
+    case PCA9552_LS1:
+    case PCA9552_LS2:
+    case PCA9552_LS3:
+        s->regs[reg] = s->buf[0];
+        pca9552_update_pin_input(s);
+        break;
+
+    case PCA9552_INPUT0:
+    case PCA9552_INPUT1:
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to register %d\n",
+                      __func__, reg);
+    }
+}
+
+static int pca9552_recv(I2CSlave *i2c)
+{
+    PCA9552State *s = PCA9552(i2c);
+
+    if (s->len < sizeof(s->buf)) {
+        return s->buf[s->len++];
+    } else {
+        return 0xff;
+    }
+}
+
+static int pca9552_send(I2CSlave *i2c, uint8_t data)
+{
+    PCA9552State *s = PCA9552(i2c);
+
+    if (s->len == 0) {
+        s->pointer = data;
+        s->len++;
+    } else {
+        if (s->len <= sizeof(s->buf)) {
+            s->buf[s->len - 1] = data;
+        }
+        s->len++;
+        pca9552_write(s);
+    }
+
+    return 0;
+}
+
+static int pca9552_event(I2CSlave *i2c, enum i2c_event event)
+{
+    PCA9552State *s = PCA9552(i2c);
+
+    if (event == I2C_START_RECV) {
+        pca9552_read(s);
+    }
+
+    s->len = 0;
+    return 0;
+}
+
+static const VMStateDescription pca9552_vmstate = {
+    .name = "PCA9552",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(len, PCA9552State),
+        VMSTATE_UINT8(pointer, PCA9552State),
+        VMSTATE_UINT8_ARRAY(buf, PCA9552State, 1),
+        VMSTATE_UINT8_ARRAY(regs, PCA9552State, PCA9552_NR_REGS),
+        VMSTATE_I2C_SLAVE(i2c, PCA9552State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void pca9552_reset(DeviceState *dev)
+{
+    PCA9552State *s = PCA9552(dev);
+
+    s->regs[PCA9552_PSC0] = 0xFF;
+    s->regs[PCA9552_PWM0] = 0x80;
+    s->regs[PCA9552_PSC1] = 0xFF;
+    s->regs[PCA9552_PWM1] = 0x80;
+    s->regs[PCA9552_LS0] = 0x55; /* all OFF */
+    s->regs[PCA9552_LS1] = 0x55;
+    s->regs[PCA9552_LS2] = 0x55;
+    s->regs[PCA9552_LS3] = 0x55;
+
+    pca9552_update_pin_input(s);
+}
+
+static void pca9552_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+    k->event = pca9552_event;
+    k->recv = pca9552_recv;
+    k->send = pca9552_send;
+    dc->reset = pca9552_reset;
+    dc->vmsd = &pca9552_vmstate;
+}
+
+static const TypeInfo pca9552_info = {
+    .name          = TYPE_PCA9552,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(PCA9552State),
+    .class_init    = pca9552_class_init,
+};
+
+static void pca9552_register_types(void)
+{
+    type_register_static(&pca9552_info);
+}
+
+type_init(pca9552_register_types)
diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
new file mode 100644
index 000000000000..6c66d4f69e9e
--- /dev/null
+++ b/include/hw/misc/pca9552.h
@@ -0,0 +1,32 @@
+/*
+ * PCA9552 I2C LED blinker
+ *
+ * Copyright (c) 2017, IBM Corporation.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+#ifndef PCA9552_H
+#define PCA9552_H
+
+#include "hw/i2c/i2c.h"
+
+#define TYPE_PCA9552 "pca9552"
+#define PCA9552(obj) OBJECT_CHECK(PCA9552State, (obj), TYPE_PCA9552)
+
+
+#define PCA9552_NR_REGS 10
+
+typedef struct PCA9552State {
+    /*< private >*/
+    I2CSlave i2c;
+    /*< public >*/
+
+    uint8_t len;
+    uint8_t pointer;
+    uint8_t buf[1];
+
+    uint8_t regs[PCA9552_NR_REGS];
+} PCA9552State;
+
+#endif
-- 
2.13.6

Re: [Qemu-devel] [PATCH v3 5/6] misc: add pca9552 LED blinker model

[Peter Maydell]

On 13 October 2017 at 15:28, Cédric Le Goater <clg@kaod.org> wrote:
> Specs are available here :
>
>   https://www.nxp.com/docs/en/data-sheet/PCA9552.pdf
>
> This is a simple model supporting the basic registers for led and GPIO
> mode. The device also supports two blinking rates but not the model
> yet.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>
>  Changes since v2:
>
>  - removed comments on the I2C buffer size, but kept the array. I did
>    not want to rewrite the buffer handling
>
>  default-configs/arm-softmmu.mak |   1 +
>  hw/misc/Makefile.objs           |   1 +
>  hw/misc/pca9552.c               | 212 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/misc/pca9552.h       |  32 ++++++
>  4 files changed, 246 insertions(+)
>  create mode 100644 hw/misc/pca9552.c
>  create mode 100644 include/hw/misc/pca9552.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 5059d134c816..d868d1095a6c 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -16,6 +16,7 @@ CONFIG_TSC2005=y
>  CONFIG_LM832X=y
>  CONFIG_TMP105=y
>  CONFIG_TMP421=y
> +CONFIG_PCA9552=y
>  CONFIG_STELLARIS=y
>  CONFIG_STELLARIS_INPUT=y
>  CONFIG_STELLARIS_ENET=y
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index e8f0a02f35af..e4e22880dbbc 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -7,6 +7,7 @@ common-obj-$(CONFIG_SGA) += sga.o
>  common-obj-$(CONFIG_ISA_TESTDEV) += pc-testdev.o
>  common-obj-$(CONFIG_PCI_TESTDEV) += pci-testdev.o
>  common-obj-$(CONFIG_EDU) += edu.o
> +common-obj-$(CONFIG_PCA9552) += pca9552.o
>
>  common-obj-y += unimp.o
>
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> new file mode 100644
> index 000000000000..22460f4c14fe
> --- /dev/null
> +++ b/hw/misc/pca9552.c
> @@ -0,0 +1,212 @@
> +/*
> + * PCA9552 I2C LED blinker
> + *
> + * Copyright (c) 2017, IBM Corporation.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later. See the COPYING file in the top-level directory.

Adding the url of the datasheet in the header comment
would also be useful.

> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/hw.h"
> +#include "hw/misc/pca9552.h"
> +
> +#define PCA9552_INPUT0   0 /* read only input register 0 */
> +#define PCA9552_INPUT1   1 /* read only input register 1  */
> +#define PCA9552_PSC0     2 /* read/write frequency prescaler 0 */
> +#define PCA9552_PWM0     3 /* read/write PWM register 0 */
> +#define PCA9552_PSC1     4 /* read/write frequency prescaler 1 */
> +#define PCA9552_PWM1     5 /* read/write PWM register 1 */
> +#define PCA9552_LS0      6 /* read/write LED0 to LED3 selector */
> +#define PCA9552_LS1      7 /* read/write LED4 to LED7 selector */
> +#define PCA9552_LS2      8 /* read/write LED8 to LED11 selector */
> +#define PCA9552_LS3      9 /* read/write LED12 to LED15 selector */
> +
> +#define PCA9552_LED_ON   0x0
> +#define PCA9552_LED_OFF  0x1
> +#define PCA9552_LED_PWM0 0x2
> +#define PCA9552_LED_PWM1 0x3
> +
> +static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
> +{
> +    uint8_t reg   = PCA9552_LS0 + (pin / 4);
> +    uint8_t shift = (pin % 4) << 1;
> +
> +    return (s->regs[reg] >> shift) & 0x3;

extract32() is probably more readable than handcoded
shift-and-mask.

> +}
> +
> +static void pca9552_update_pin_input(PCA9552State *s)
> +{
> +    int i;
> +
> +    for (i = 0; i < 16; i++) {
> +        uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
> +        uint8_t input_shift = (i % 8);
> +        uint8_t config = pca9552_pin_get_config(s, i);
> +
> +        switch (config) {
> +        case PCA9552_LED_ON:
> +            s->regs[input_reg] |= 1 << input_shift;
> +            break;
> +        case PCA9552_LED_OFF:
> +            s->regs[input_reg] &= ~(1 << input_shift);
> +            break;
> +        case PCA9552_LED_PWM0:
> +        case PCA9552_LED_PWM1:
> +            /* ??? */
> +        default:
> +            break;
> +        }
> +    }
> +}
> +
> +static void pca9552_read(PCA9552State *s)
> +{
> +    uint8_t reg = s->pointer & 0xf;
> +
> +    s->len = 0;
> +
> +    switch (reg) {
> +    case PCA9552_INPUT0:
> +    case PCA9552_INPUT1:
> +    case PCA9552_PSC0:
> +    case PCA9552_PWM0:
> +    case PCA9552_PSC1:
> +    case PCA9552_PWM1:
> +    case PCA9552_LS0:
> +    case PCA9552_LS1:
> +    case PCA9552_LS2:
> +    case PCA9552_LS3:
> +        s->buf[s->len++] = s->regs[reg];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to register %d\n",
> +                      __func__, reg);
> +    }
> +}
> +
> +static void pca9552_write(PCA9552State *s)
> +{
> +    uint8_t reg = s->pointer & 0xf;
> +
> +    switch (reg) {
> +    case PCA9552_PSC0:
> +    case PCA9552_PWM0:
> +    case PCA9552_PSC1:
> +    case PCA9552_PWM1:
> +        s->regs[reg] = s->buf[0];
> +        break;
> +
> +    case PCA9552_LS0:
> +    case PCA9552_LS1:
> +    case PCA9552_LS2:
> +    case PCA9552_LS3:
> +        s->regs[reg] = s->buf[0];
> +        pca9552_update_pin_input(s);
> +        break;
> +
> +    case PCA9552_INPUT0:
> +    case PCA9552_INPUT1:
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to register %d\n",
> +                      __func__, reg);
> +    }
> +}
> +
> +static int pca9552_recv(I2CSlave *i2c)
> +{
> +    PCA9552State *s = PCA9552(i2c);
> +
> +    if (s->len < sizeof(s->buf)) {
> +        return s->buf[s->len++];
> +    } else {
> +        return 0xff;
> +    }
> +}
> +
> +static int pca9552_send(I2CSlave *i2c, uint8_t data)
> +{
> +    PCA9552State *s = PCA9552(i2c);
> +
> +    if (s->len == 0) {
> +        s->pointer = data;
> +        s->len++;
> +    } else {
> +        if (s->len <= sizeof(s->buf)) {
> +            s->buf[s->len - 1] = data;
> +        }
> +        s->len++;
> +        pca9552_write(s);
> +    }
> +
> +    return 0;
> +}
> +
> +static int pca9552_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    PCA9552State *s = PCA9552(i2c);
> +
> +    if (event == I2C_START_RECV) {
> +        pca9552_read(s);
> +    }
> +
> +    s->len = 0;
> +    return 0;
> +}

Reading the data sheet, it doesn't look like this is
correctly implementing reads and writes of more than one byte.
If you look at figures 11, 12 and 13 the guest can:
 * read or write multiple registers at once with a
   single transaction with multiple bytes, using the
   autoincrement (AI) bit in the command byte
 * read or writes multiple bytes of data from a port
   configured for GPIO with a single transaction
   (in this case AI would be clear)

There's a clearer description in the application note:
https://www.nxp.com/docs/en/application-note/AN264.pdf
(on page 12).

I think to implement this you don't need buf[] at all
(and len is only there to distinguish "first byte" from
"not first byte").

Rather than calling pca9552_read() from pca9552_event()
you should call it from pca9552_recv(), which means that
it gets called for each byte requested and you don't
need to stuff the value into buf[] and then fish it
back out again.

Similarly, rather than pca9552_send() writing the data
into s->buf[] and then pca9552_write() fishing it out,
you can just pass it directly to pca9552_write().

In both cases you want to implement the "increment
pointer as required if AI bit is set" so multibyte
transfers step through the register set, rolling over
from 9 to 0.

I don't think the Linux driver bothers to use this, but it's
worth getting it right from the start because it affects how
we represent the device state and thus migration compat.

> +static const VMStateDescription pca9552_vmstate = {
> +    .name = "PCA9552",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(len, PCA9552State),
> +        VMSTATE_UINT8(pointer, PCA9552State),
> +        VMSTATE_UINT8_ARRAY(buf, PCA9552State, 1),
> +        VMSTATE_UINT8_ARRAY(regs, PCA9552State, PCA9552_NR_REGS),
> +        VMSTATE_I2C_SLAVE(i2c, PCA9552State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void pca9552_reset(DeviceState *dev)
> +{
> +    PCA9552State *s = PCA9552(dev);
> +
> +    s->regs[PCA9552_PSC0] = 0xFF;
> +    s->regs[PCA9552_PWM0] = 0x80;
> +    s->regs[PCA9552_PSC1] = 0xFF;
> +    s->regs[PCA9552_PWM1] = 0x80;
> +    s->regs[PCA9552_LS0] = 0x55; /* all OFF */
> +    s->regs[PCA9552_LS1] = 0x55;
> +    s->regs[PCA9552_LS2] = 0x55;
> +    s->regs[PCA9552_LS3] = 0x55;
> +
> +    pca9552_update_pin_input(s);

Don't we also need to reset the pointer and len fields?


thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 5/6] misc: add pca9552 LED blinker model

[Cédric Le Goater]

On 10/17/2017 07:13 PM, Peter Maydell wrote:
> On 13 October 2017 at 15:28, Cédric Le Goater <clg@kaod.org> wrote:
>> Specs are available here :
>>
>>   https://www.nxp.com/docs/en/data-sheet/PCA9552.pdf
>>
>> This is a simple model supporting the basic registers for led and GPIO
>> mode. The device also supports two blinking rates but not the model
>> yet.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  Changes since v2:
>>
>>  - removed comments on the I2C buffer size, but kept the array. I did
>>    not want to rewrite the buffer handling
>>
>>  default-configs/arm-softmmu.mak |   1 +
>>  hw/misc/Makefile.objs           |   1 +
>>  hw/misc/pca9552.c               | 212 ++++++++++++++++++++++++++++++++++++++++
>>  include/hw/misc/pca9552.h       |  32 ++++++
>>  4 files changed, 246 insertions(+)
>>  create mode 100644 hw/misc/pca9552.c
>>  create mode 100644 include/hw/misc/pca9552.h
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index 5059d134c816..d868d1095a6c 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -16,6 +16,7 @@ CONFIG_TSC2005=y
>>  CONFIG_LM832X=y
>>  CONFIG_TMP105=y
>>  CONFIG_TMP421=y
>> +CONFIG_PCA9552=y
>>  CONFIG_STELLARIS=y
>>  CONFIG_STELLARIS_INPUT=y
>>  CONFIG_STELLARIS_ENET=y
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index e8f0a02f35af..e4e22880dbbc 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -7,6 +7,7 @@ common-obj-$(CONFIG_SGA) += sga.o
>>  common-obj-$(CONFIG_ISA_TESTDEV) += pc-testdev.o
>>  common-obj-$(CONFIG_PCI_TESTDEV) += pci-testdev.o
>>  common-obj-$(CONFIG_EDU) += edu.o
>> +common-obj-$(CONFIG_PCA9552) += pca9552.o
>>
>>  common-obj-y += unimp.o
>>
>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>> new file mode 100644
>> index 000000000000..22460f4c14fe
>> --- /dev/null
>> +++ b/hw/misc/pca9552.c
>> @@ -0,0 +1,212 @@
>> +/*
>> + * PCA9552 I2C LED blinker
>> + *
>> + * Copyright (c) 2017, IBM Corporation.
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later. See the COPYING file in the top-level directory.
> 
> Adding the url of the datasheet in the header comment
> would also be useful.

yes.


> 
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "hw/hw.h"
>> +#include "hw/misc/pca9552.h"
>> +
>> +#define PCA9552_INPUT0   0 /* read only input register 0 */
>> +#define PCA9552_INPUT1   1 /* read only input register 1  */
>> +#define PCA9552_PSC0     2 /* read/write frequency prescaler 0 */
>> +#define PCA9552_PWM0     3 /* read/write PWM register 0 */
>> +#define PCA9552_PSC1     4 /* read/write frequency prescaler 1 */
>> +#define PCA9552_PWM1     5 /* read/write PWM register 1 */
>> +#define PCA9552_LS0      6 /* read/write LED0 to LED3 selector */
>> +#define PCA9552_LS1      7 /* read/write LED4 to LED7 selector */
>> +#define PCA9552_LS2      8 /* read/write LED8 to LED11 selector */
>> +#define PCA9552_LS3      9 /* read/write LED12 to LED15 selector */
>> +
>> +#define PCA9552_LED_ON   0x0
>> +#define PCA9552_LED_OFF  0x1
>> +#define PCA9552_LED_PWM0 0x2
>> +#define PCA9552_LED_PWM1 0x3
>> +
>> +static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
>> +{
>> +    uint8_t reg   = PCA9552_LS0 + (pin / 4);
>> +    uint8_t shift = (pin % 4) << 1;
>> +
>> +    return (s->regs[reg] >> shift) & 0x3;
> 
> extract32() is probably more readable than handcoded
> shift-and-mask.
> 

ok


>> +}
>> +
>> +static void pca9552_update_pin_input(PCA9552State *s)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < 16; i++) {
>> +        uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
>> +        uint8_t input_shift = (i % 8);
>> +        uint8_t config = pca9552_pin_get_config(s, i);
>> +
>> +        switch (config) {
>> +        case PCA9552_LED_ON:
>> +            s->regs[input_reg] |= 1 << input_shift;
>> +            break;
>> +        case PCA9552_LED_OFF:
>> +            s->regs[input_reg] &= ~(1 << input_shift);
>> +            break;
>> +        case PCA9552_LED_PWM0:
>> +        case PCA9552_LED_PWM1:
>> +            /* ??? */
>> +        default:
>> +            break;
>> +        }
>> +    }
>> +}
>> +
>> +static void pca9552_read(PCA9552State *s)
>> +{
>> +    uint8_t reg = s->pointer & 0xf;
>> +
>> +    s->len = 0;
>> +
>> +    switch (reg) {
>> +    case PCA9552_INPUT0:
>> +    case PCA9552_INPUT1:
>> +    case PCA9552_PSC0:
>> +    case PCA9552_PWM0:
>> +    case PCA9552_PSC1:
>> +    case PCA9552_PWM1:
>> +    case PCA9552_LS0:
>> +    case PCA9552_LS1:
>> +    case PCA9552_LS2:
>> +    case PCA9552_LS3:
>> +        s->buf[s->len++] = s->regs[reg];
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected read to register %d\n",
>> +                      __func__, reg);
>> +    }
>> +}
>> +
>> +static void pca9552_write(PCA9552State *s)
>> +{
>> +    uint8_t reg = s->pointer & 0xf;
>> +
>> +    switch (reg) {
>> +    case PCA9552_PSC0:
>> +    case PCA9552_PWM0:
>> +    case PCA9552_PSC1:
>> +    case PCA9552_PWM1:
>> +        s->regs[reg] = s->buf[0];
>> +        break;
>> +
>> +    case PCA9552_LS0:
>> +    case PCA9552_LS1:
>> +    case PCA9552_LS2:
>> +    case PCA9552_LS3:
>> +        s->regs[reg] = s->buf[0];
>> +        pca9552_update_pin_input(s);
>> +        break;
>> +
>> +    case PCA9552_INPUT0:
>> +    case PCA9552_INPUT1:
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unexpected write to register %d\n",
>> +                      __func__, reg);
>> +    }
>> +}
>> +
>> +static int pca9552_recv(I2CSlave *i2c)
>> +{
>> +    PCA9552State *s = PCA9552(i2c);
>> +
>> +    if (s->len < sizeof(s->buf)) {
>> +        return s->buf[s->len++];
>> +    } else {
>> +        return 0xff;
>> +    }
>> +}
>> +
>> +static int pca9552_send(I2CSlave *i2c, uint8_t data)
>> +{
>> +    PCA9552State *s = PCA9552(i2c);
>> +
>> +    if (s->len == 0) {
>> +        s->pointer = data;
>> +        s->len++;
>> +    } else {
>> +        if (s->len <= sizeof(s->buf)) {
>> +            s->buf[s->len - 1] = data;
>> +        }
>> +        s->len++;
>> +        pca9552_write(s);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int pca9552_event(I2CSlave *i2c, enum i2c_event event)
>> +{
>> +    PCA9552State *s = PCA9552(i2c);
>> +
>> +    if (event == I2C_START_RECV) {
>> +        pca9552_read(s);
>> +    }
>> +
>> +    s->len = 0;
>> +    return 0;
>> +}
> 
> Reading the data sheet, it doesn't look like this is
> correctly implementing reads and writes of more than one byte.
> If you look at figures 11, 12 and 13 the guest can:
>  * read or write multiple registers at once with a
>    single transaction with multiple bytes, using the
>    autoincrement (AI) bit in the command byte
>  * read or writes multiple bytes of data from a port
>    configured for GPIO with a single transaction
>    (in this case AI would be clear)

I completely overlooked the AI support but it does not 
seem too complex to implement.

> There's a clearer description in the application note:
> https://www.nxp.com/docs/en/application-note/AN264.pdf
> (on page 12).

This is a much better document than the one I had found ...

> I think to implement this you don't need buf[] at all
> (and len is only there to distinguish "first byte" from
> "not first byte").
> 
> Rather than calling pca9552_read() from pca9552_event()
> you should call it from pca9552_recv(), which means that
> it gets called for each byte requested and you don't
> need to stuff the value into buf[] and then fish it
> back out again.
> 
> Similarly, rather than pca9552_send() writing the data
> into s->buf[] and then pca9552_write() fishing it out,
> you can just pass it directly to pca9552_write().

Yes. These are all good cleanups. I suspect other I2C models
would also benefit from your suggestions. I will take a look
later on.

> In both cases you want to implement the "increment
> pointer as required if AI bit is set" so multibyte
> transfers step through the register set, rolling over
> from 9 to 0.

I will send an updated version with AI support in the next 
round. 

> I don't think the Linux driver bothers to use this, but it's
> worth getting it right from the start because it affects how
> we represent the device state and thus migration compat.

yes.

>> +static const VMStateDescription pca9552_vmstate = {
>> +    .name = "PCA9552",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(len, PCA9552State),
>> +        VMSTATE_UINT8(pointer, PCA9552State),
>> +        VMSTATE_UINT8_ARRAY(buf, PCA9552State, 1),
>> +        VMSTATE_UINT8_ARRAY(regs, PCA9552State, PCA9552_NR_REGS),
>> +        VMSTATE_I2C_SLAVE(i2c, PCA9552State),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void pca9552_reset(DeviceState *dev)
>> +{
>> +    PCA9552State *s = PCA9552(dev);
>> +
>> +    s->regs[PCA9552_PSC0] = 0xFF;
>> +    s->regs[PCA9552_PWM0] = 0x80;
>> +    s->regs[PCA9552_PSC1] = 0xFF;
>> +    s->regs[PCA9552_PWM1] = 0x80;
>> +    s->regs[PCA9552_LS0] = 0x55; /* all OFF */
>> +    s->regs[PCA9552_LS1] = 0x55;
>> +    s->regs[PCA9552_LS2] = 0x55;
>> +    s->regs[PCA9552_LS3] = 0x55;
>> +
>> +    pca9552_update_pin_input(s);
> 
> Don't we also need to reset the pointer and len fields?
> 

yes. indeed.

Thanks,

C. 


> thanks
> -- PMM
> 

[Qemu-devel] [PATCH v3 6/6] aspeed: add the pc9552 chips to the witherspoon machine

[Cédric Le Goater]

The pca9552 LED blinkers on the Witherspoon machine are used for leds
but also as GPIOs to control fans and GPUs.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/arm/aspeed.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 2a6ca58527f8..f3c279cfcd3f 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -379,6 +379,8 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
     AspeedSoCState *soc = &bmc->soc;
     uint8_t *eeprom_buf = g_malloc0(8 * 1024);
 
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3), "pca9552", 0x60);
+
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 0x4c);
     i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 0x4c);
 
@@ -391,6 +393,8 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
 
     smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), 0x51,
                           eeprom_buf);
+    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 11), "pca9552",
+                     0x60);
 }
 
 static void witherspoon_bmc_init(MachineState *machine)
-- 
2.13.6