| Message ID | 20200616094542.25415-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/misc/pca9552: Trace LEDs events, make LEDs 13-15 as GPIOs | expand |
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" >>> >> > >
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"
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/misc/pca9552.c | 7 +++++++ hw/misc/trace-events | 3 +++ 2 files changed, 10 insertions(+)