On 10/17/2017 07:13 PM, Peter Maydell wrote:
> On 13 October 2017 at 15:28, Cédric Le Goater <c...@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 <c...@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
>
