Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/misc/pca9552.c | 7 +++++++
hw/misc/trace-events | 3 +++
2 files changed, 10 insertions(+)
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index cac729e35a..a3d0decbff 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -17,6 +17,7 @@
#include "migration/vmstate.h"
#include "qapi/error.h"
#include "qapi/visitor.h"
+#include "trace.h"
#define PCA9552_LED_ON 0x0
#define PCA9552_LED_OFF 0x1
@@ -45,9 +46,15 @@ static void pca9552_update_pin_input(PCA9552State *s)
switch (config) {
case PCA9552_LED_ON:
s->regs[input_reg] |= 1 << input_shift;
+ if (input_shift < s->nr_leds) {
+ trace_pca9552_led_set(input_shift, true);
+ }
break;
case PCA9552_LED_OFF:
s->regs[input_reg] &= ~(1 << input_shift);
+ if (input_shift < s->nr_leds) {
+ trace_pca9552_led_set(input_shift, false);
+ }
break;
case PCA9552_LED_PWM0:
case PCA9552_LED_PWM1:
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 5561746866..ed80d0d1be 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
# grlib_ahb_apb_pnp.c
grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
+
+# pca9552.c
+pca9552_led_set(unsigned id, bool state) "LED#%d state:%u"
--
2.21.3
On 6/16/20 11:45 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>
Download an OpenBMC witherspoon flash image :
wget https://openpower.xyz/job/openbmc-build/lastSuccessfulBuild/distro=ubuntu,label=builder,target=witherspoon/artifact/deploy/images/witherspoon/obmc-phosphor-image-witherspoon.ubi.mtd
and run :
qemu-system-arm -M witherspoon-bmc -nic user -drive file=obmc-phosphor-image-witherspoon.ubi.mtd,format=raw,if=mtd -nographic
The system has a set of daemons controlling the status of the leds :
1148 root 7164 S phosphor-ledcontroller -p /sys/class/leds/cffps1/68
1149 root 7164 S phosphor-ledcontroller -p /sys/class/leds/cffps1/69
1150 root 7164 S phosphor-ledcontroller -p /sys/class/leds/fan0
1151 root 7164 S phosphor-ledcontroller -p /sys/class/leds/fan1
1152 root 7164 S phosphor-ledcontroller -p /sys/class/leds/fan2
1153 root 7164 S phosphor-ledcontroller -p /sys/class/leds/fan3
1154 root 7164 S phosphor-ledcontroller -p /sys/class/leds/front/fault
1155 root 7164 S phosphor-ledcontroller -p /sys/class/leds/front/id
1156 root 7164 S phosphor-ledcontroller -p /sys/class/leds/front/power
1157 root 7164 S phosphor-ledcontroller -p /sys/class/leds/power/button
1158 root 7164 S phosphor-ledcontroller -p /sys/class/leds/rear/fault
1159 root 7164 S phosphor-ledcontroller -p /sys/class/leds/rear/id
1160 root 7164 S phosphor-ledcontroller -p /sys/class/leds/rear/power
If activated, the pca9552 traces will appear on the QEMU monitor.
Thanks,
C.
> ---
> hw/misc/pca9552.c | 7 +++++++
> hw/misc/trace-events | 3 +++
> 2 files changed, 10 insertions(+)
>
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index cac729e35a..a3d0decbff 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -17,6 +17,7 @@
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> #include "qapi/visitor.h"
> +#include "trace.h"
>
> #define PCA9552_LED_ON 0x0
> #define PCA9552_LED_OFF 0x1
> @@ -45,9 +46,15 @@ static void pca9552_update_pin_input(PCA9552State *s)
> switch (config) {
> case PCA9552_LED_ON:
> s->regs[input_reg] |= 1 << input_shift;
> + if (input_shift < s->nr_leds) {
> + trace_pca9552_led_set(input_shift, true);
> + }
> break;
> case PCA9552_LED_OFF:
> s->regs[input_reg] &= ~(1 << input_shift);
> + if (input_shift < s->nr_leds) {
> + trace_pca9552_led_set(input_shift, false);
> + }
> break;
> case PCA9552_LED_PWM0:
> case PCA9552_LED_PWM1:
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 5561746866..ed80d0d1be 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
> # grlib_ahb_apb_pnp.c
> grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
> grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
> +
> +# pca9552.c
> +pca9552_led_set(unsigned id, bool state) "LED#%d state:%u"
>
After a closer look,
>> @@ -45,9 +46,15 @@ static void pca9552_update_pin_input(PCA9552State *s)
>> switch (config) {
>> case PCA9552_LED_ON:
>> s->regs[input_reg] |= 1 << input_shift;
>> + if (input_shift < s->nr_leds) {
This seems like a superfluous test.
>> + trace_pca9552_led_set(input_shift, true);
This should be using 'i' and the output is not very concise :
564609@1592326881.815616:pca9552_led_set LED#0 state:1
564609@1592326881.815660:pca9552_led_set LED#1 state:1
564609@1592326881.815669:pca9552_led_set LED#2 state:1
564609@1592326881.815677:pca9552_led_set LED#3 state:1
564609@1592326881.815706:pca9552_led_set LED#4 state:0
564609@1592326881.815715:pca9552_led_set LED#5 state:0
564609@1592326881.815727:pca9552_led_set LED#6 state:0
564609@1592326881.815740:pca9552_led_set LED#7 state:0
564609@1592326881.815748:pca9552_led_set LED#8 state:0
564609@1592326881.815759:pca9552_led_set LED#9 state:0
564609@1592326881.815767:pca9552_led_set LED#10 state:0
564609@1592326881.815779:pca9552_led_set LED#11 state:0
564609@1592326881.815790:pca9552_led_set LED#12 state:0
564609@1592326881.815802:pca9552_led_set LED#13 state:1
564609@1592326881.815813:pca9552_led_set LED#14 state:1
564609@1592326881.815826:pca9552_led_set LED#15 state:1
I would instead simply dump the contents of the PCA9552_INPUT registers :
LEDS = 1111000000000101
LEDS = 1111000000000111
....
C.
>> + }
>> break;
>> case PCA9552_LED_OFF:
>> s->regs[input_reg] &= ~(1 << input_shift);
>> + if (input_shift < s->nr_leds) {
>> + trace_pca9552_led_set(input_shift, false);
>> + }
>> break;
>> case PCA9552_LED_PWM0:
>> case PCA9552_LED_PWM1:
>> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
>> index 5561746866..ed80d0d1be 100644
>> --- a/hw/misc/trace-events
>> +++ b/hw/misc/trace-events
>> @@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
>> # grlib_ahb_apb_pnp.c
>> grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
>> grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
>> +
>> +# pca9552.c
>> +pca9552_led_set(unsigned id, bool state) "LED#%d state:%u"
>>
>
On 6/16/20 7:15 PM, Cédric Le Goater wrote:
> After a closer look,
>
>>> @@ -45,9 +46,15 @@ static void pca9552_update_pin_input(PCA9552State *s)
>>> switch (config) {
>>> case PCA9552_LED_ON:
>>> s->regs[input_reg] |= 1 << input_shift;
>>> + if (input_shift < s->nr_leds) {
> This seems like a superfluous test.
Indeed.
>
>>> + trace_pca9552_led_set(input_shift, true);
>
> This should be using 'i' and the output is not very concise :
>
> 564609@1592326881.815616:pca9552_led_set LED#0 state:1
> 564609@1592326881.815660:pca9552_led_set LED#1 state:1
> 564609@1592326881.815669:pca9552_led_set LED#2 state:1
> 564609@1592326881.815677:pca9552_led_set LED#3 state:1
> 564609@1592326881.815706:pca9552_led_set LED#4 state:0
> 564609@1592326881.815715:pca9552_led_set LED#5 state:0
> 564609@1592326881.815727:pca9552_led_set LED#6 state:0
> 564609@1592326881.815740:pca9552_led_set LED#7 state:0
> 564609@1592326881.815748:pca9552_led_set LED#8 state:0
> 564609@1592326881.815759:pca9552_led_set LED#9 state:0
> 564609@1592326881.815767:pca9552_led_set LED#10 state:0
> 564609@1592326881.815779:pca9552_led_set LED#11 state:0
> 564609@1592326881.815790:pca9552_led_set LED#12 state:0
> 564609@1592326881.815802:pca9552_led_set LED#13 state:1
> 564609@1592326881.815813:pca9552_led_set LED#14 state:1
> 564609@1592326881.815826:pca9552_led_set LED#15 state:1
>
> I would instead simply dump the contents of the PCA9552_INPUT registers :
>
> LEDS = 1111000000000101
> LEDS = 1111000000000111
> ....
Good idea, thanks!
>
> C.
>
>
>>> + }
>>> break;
>>> case PCA9552_LED_OFF:
>>> s->regs[input_reg] &= ~(1 << input_shift);
>>> + if (input_shift < s->nr_leds) {
>>> + trace_pca9552_led_set(input_shift, false);
>>> + }
>>> break;
>>> case PCA9552_LED_PWM0:
>>> case PCA9552_LED_PWM1:
>>> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
>>> index 5561746866..ed80d0d1be 100644
>>> --- a/hw/misc/trace-events
>>> +++ b/hw/misc/trace-events
>>> @@ -206,3 +206,6 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
>>> # grlib_ahb_apb_pnp.c
>>> grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
>>> grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read addr:0x%03"PRIx64" data:0x%08x"
>>> +
>>> +# pca9552.c
>>> +pca9552_led_set(unsigned id, bool state) "LED#%d state:%u"
>>>
>>
>
>
© 2016 - 2026 Red Hat, Inc.