| Message ID | 20230204174758.234951-6-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | pcie: cleanup code and add trace point | expand |
On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > hw/pci/pcie.c | 20 ++++++++++++++++++++ > hw/pci/trace-events | 3 +++ > 2 files changed, 23 insertions(+) > +static const char *pcie_sltctl_pic_str(uint16_t sltctl) > +{ > + switch (sltctl & PCI_EXP_SLTCTL_PIC) { > + case PCI_EXP_SLTCTL_PWR_IND_ON: > + return "on"; > + case PCI_EXP_SLTCTL_PWR_IND_BLINK: > + return "blink"; > + case PCI_EXP_SLTCTL_PWR_IND_OFF: > + return "off"; > + default: > + return "?"; Maybe "illegal"? Otherwise: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > + } > +}
Thanks for reviewing! On 05.02.23 13:56, Philippe Mathieu-Daudé wrote: > On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote: >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> --- >> hw/pci/pcie.c | 20 ++++++++++++++++++++ >> hw/pci/trace-events | 3 +++ >> 2 files changed, 23 insertions(+) > >> +static const char *pcie_sltctl_pic_str(uint16_t sltctl) >> +{ >> + switch (sltctl & PCI_EXP_SLTCTL_PIC) { >> + case PCI_EXP_SLTCTL_PWR_IND_ON: >> + return "on"; >> + case PCI_EXP_SLTCTL_PWR_IND_BLINK: >> + return "blink"; >> + case PCI_EXP_SLTCTL_PWR_IND_OFF: >> + return "off"; >> + default: >> + return "?"; > > Maybe "illegal"? I just was unsure about it. For SHPC, 0 is correct, and means that this command don't change the led state. But with PCI-e hotplug we don't have such commands but change the led directly, so it must be one of "on"/"blink"/"off", and zero is really wrong, right? Also, I'm now looking at /* TODO: send event to monitor */ in shpc code, and working on it. So, I think, I'll soon send patches with such event for both SHPC and PCI-e, and probably that trace point becomes not needed. > > Otherwise: > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >> + } >> +} >
On Tue, Feb 07, 2023 at 01:39:03PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Thanks for reviewing! > > On 05.02.23 13:56, Philippe Mathieu-Daudé wrote: > > On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote: > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > --- > > > hw/pci/pcie.c | 20 ++++++++++++++++++++ > > > hw/pci/trace-events | 3 +++ > > > 2 files changed, 23 insertions(+) > > > > > +static const char *pcie_sltctl_pic_str(uint16_t sltctl) > > > +{ > > > + switch (sltctl & PCI_EXP_SLTCTL_PIC) { > > > + case PCI_EXP_SLTCTL_PWR_IND_ON: > > > + return "on"; > > > + case PCI_EXP_SLTCTL_PWR_IND_BLINK: > > > + return "blink"; > > > + case PCI_EXP_SLTCTL_PWR_IND_OFF: > > > + return "off"; > > > + default: > > > + return "?"; > > > > Maybe "illegal"? > > I just was unsure about it. > > For SHPC, 0 is correct, and means that this command don't change the led state. > > But with PCI-e hotplug we don't have such commands but change the led directly, so it must be one of "on"/"blink"/"off", and zero is really wrong, right? > > > Also, I'm now looking at /* TODO: send event to monitor */ in shpc code, and working on it. So, I think, I'll soon send patches with such event for both SHPC and PCI-e, and probably that trace point becomes not needed. I think it's ok to queue as is, change it with a patch on top. > > > > Otherwise: > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > > + } > > > +} > > > > -- > Best regards, > Vladimir
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index ccdb2377e1..1a19368994 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -28,6 +28,7 @@ #include "hw/pci/pcie_regs.h" #include "hw/pci/pcie_port.h" #include "qemu/range.h" +#include "trace.h" //#define DEBUG_PCIE #ifdef DEBUG_PCIE @@ -718,6 +719,20 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); } +static const char *pcie_sltctl_pic_str(uint16_t sltctl) +{ + switch (sltctl & PCI_EXP_SLTCTL_PIC) { + case PCI_EXP_SLTCTL_PWR_IND_ON: + return "on"; + case PCI_EXP_SLTCTL_PWR_IND_BLINK: + return "blink"; + case PCI_EXP_SLTCTL_PWR_IND_OFF: + return "off"; + default: + return "?"; + } +} + void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t old_slt_ctl, uint16_t old_slt_sta, uint32_t addr, uint32_t val, int len) @@ -762,6 +777,11 @@ void pcie_cap_slot_write_config(PCIDevice *dev, sltsta); } + if ((val & PCI_EXP_SLTCTL_PIC) != (old_slt_ctl & PCI_EXP_SLTCTL_PIC)) { + trace_pcie_power_indicator(pcie_sltctl_pic_str(old_slt_ctl), + pcie_sltctl_pic_str(val)); + } + /* * If the slot is populated, power indicator is off and power * controller is off, it is safe to detach the devices. diff --git a/hw/pci/trace-events b/hw/pci/trace-events index aaf46bc92d..ec4a5ff43d 100644 --- a/hw/pci/trace-events +++ b/hw/pci/trace-events @@ -15,3 +15,6 @@ msix_write_config(char *name, bool enabled, bool masked) "dev %s enabled %d mask sriov_register_vfs(const char *name, int slot, int function, int num_vfs) "%s %02x:%x: creating %d vf devs" sriov_unregister_vfs(const char *name, int slot, int function, int num_vfs) "%s %02x:%x: Unregistering %d vf devs" sriov_config_write(const char *name, int slot, int fun, uint32_t offset, uint32_t val, uint32_t len) "%s %02x:%x: sriov offset 0x%x val 0x%x len %d" + +# pcie.c +pcie_power_indicator(const char *old, const char *new) "%s -> %s"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- hw/pci/pcie.c | 20 ++++++++++++++++++++ hw/pci/trace-events | 3 +++ 2 files changed, 23 insertions(+)