Re: [PATCH v4 08/16] pcie_regs: drop duplicated indicator value macros

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> We already have indicator values in
> include/standard-headers/linux/pci_regs.h , no reason to reinvent them
> in include/hw/pci/pcie_regs.h. (and we already have usage of
> PCI_EXP_SLTCTL_PWR_IND_BLINK and PCI_EXP_SLTCTL_PWR_IND_OFF in
> hw/pci/pcie.c, so let's be consistent)
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
> ---
>   include/hw/pci/pcie_regs.h |  9 ---------
>   hw/pci/pcie.c              | 13 +++++++------
>   2 files changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
> index 963dc2e170..00b595a82e 100644
> --- a/include/hw/pci/pcie_regs.h
> +++ b/include/hw/pci/pcie_regs.h
> @@ -70,15 +70,6 @@ typedef enum PCIExpLinkWidth {
>   #define PCI_EXP_SLTCTL_IND_ON           0x1
>   #define PCI_EXP_SLTCTL_IND_BLINK        0x2
>   #define PCI_EXP_SLTCTL_IND_OFF          0x3
> -#define PCI_EXP_SLTCTL_AIC_SHIFT        ctz32(PCI_EXP_SLTCTL_AIC)
> -#define PCI_EXP_SLTCTL_AIC_OFF                          \
> -    (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_AIC_SHIFT)
> -
> -#define PCI_EXP_SLTCTL_PIC_SHIFT        ctz32(PCI_EXP_SLTCTL_PIC)
> -#define PCI_EXP_SLTCTL_PIC_OFF                          \
> -    (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT)
> -#define PCI_EXP_SLTCTL_PIC_ON                          \
> -    (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT)
>   
>   #define PCI_EXP_SLTCTL_SUPPORTED        \
>               (PCI_EXP_SLTCTL_ABPE |      \
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 82ef723983..ccdb2377e1 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -634,8 +634,8 @@ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
>                                    PCI_EXP_SLTCTL_PIC |
>                                    PCI_EXP_SLTCTL_AIC);
>       pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL,
> -                               PCI_EXP_SLTCTL_PIC_OFF |
> -                               PCI_EXP_SLTCTL_AIC_OFF);
> +                               PCI_EXP_SLTCTL_PWR_IND_OFF |
> +                               PCI_EXP_SLTCTL_ATTN_IND_OFF);
>       pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL,
>                                  PCI_EXP_SLTCTL_PIC |
>                                  PCI_EXP_SLTCTL_AIC |
> @@ -679,7 +679,7 @@ void pcie_cap_slot_reset(PCIDevice *dev)
>                                    PCI_EXP_SLTCTL_PDCE |
>                                    PCI_EXP_SLTCTL_ABPE);
>       pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL,
> -                               PCI_EXP_SLTCTL_AIC_OFF);
> +                               PCI_EXP_SLTCTL_ATTN_IND_OFF);
>   
>       if (dev->cap_present & QEMU_PCIE_SLTCAP_PCP) {
>           /* Downstream ports enforce device number 0. */
> @@ -694,7 +694,8 @@ void pcie_cap_slot_reset(PCIDevice *dev)
>                                          PCI_EXP_SLTCTL_PCC);
>           }
>   
> -        pic = populated ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF;
> +        pic = populated ?
> +                PCI_EXP_SLTCTL_PWR_IND_ON : PCI_EXP_SLTCTL_PWR_IND_OFF;
>           pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, pic);
>       }
>   
> @@ -770,9 +771,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>        * control of powered off slots before powering them on.
>        */
>       if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
> -        (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PIC_OFF &&
> +        (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF &&
>           (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
> -        (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PIC_OFF)) {
> +        (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PWR_IND_OFF)) {
>           pcie_cap_slot_do_unplug(dev);
>       }
>       pcie_cap_update_power(dev);