Re: [Qemu-arm] [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 <address@hidden> 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 <address@hidden>
>> ---
>>
>>  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
>