Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality

[Michael S. Tsirkin]

On Thu, Jun 19, 2014 at 04:52:20PM +0300, Marcel Apfelbaum wrote:
> It is needed by hot-unplug in order to get an indication
> from the OS when the device can be physically detached.
> 
> Signed-off-by: Marcel Apfelbaum <address@hidden>
> ---
>  hw/pci/pcie.c              | 15 ++++++++++++++-
>  include/hw/pci/pcie_regs.h |  2 ++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index ae92f00..f8bf515 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -292,16 +292,19 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
>                                 PCI_EXP_SLTCAP_HPC |
>                                 PCI_EXP_SLTCAP_PIP |
>                                 PCI_EXP_SLTCAP_AIP |
> +                               PCI_EXP_SLTCAP_PCP |
>                                 PCI_EXP_SLTCAP_ABP);
>  
>      pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL,
>                                   PCI_EXP_SLTCTL_PIC |
> +                                 PCI_EXP_SLTCTL_PCC |
>                                   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_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL,
>                                 PCI_EXP_SLTCTL_PIC |
> +                               PCI_EXP_SLTCTL_PCC |
>                                 PCI_EXP_SLTCTL_AIC |
>                                 PCI_EXP_SLTCTL_HPIE |
>                                 PCI_EXP_SLTCTL_CCIE |

Need to disable for compat types?


> @@ -327,21 +330,31 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
>  void pcie_cap_slot_reset(PCIDevice *dev)
>  {
>      uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
> +    PCIDevice *slot_dev = 
> pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0];

What does this mean?
Downstream port?
A switch can have multiple downstream ports at any slot #.

> +    int pic;

uint16_t please.

> +
> +    pic = slot_dev ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF;
>  
>      PCIE_DEV_PRINTF(dev, "reset\n");
>  
>      pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL,
>                                   PCI_EXP_SLTCTL_EIC |
>                                   PCI_EXP_SLTCTL_PIC |
> +                                 PCI_EXP_SLTCTL_PCC |
>                                   PCI_EXP_SLTCTL_AIC |
>                                   PCI_EXP_SLTCTL_HPIE |
>                                   PCI_EXP_SLTCTL_CCIE |
>                                   PCI_EXP_SLTCTL_PDCE |
>                                   PCI_EXP_SLTCTL_ABPE);
>      pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL,
> -                               PCI_EXP_SLTCTL_PIC_OFF |
> +                               pic |
>                                 PCI_EXP_SLTCTL_AIC_OFF);
>  
> +    if (!slot_dev) {
> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL,
> +                                   PCI_EXP_SLTCTL_PCC);

I dislike it when we clear bits then set them back.
Please just add else here.

> +    }
> +

Need to disable for compat types?

>      pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
>                                   PCI_EXP_SLTSTA_EIS |/* on reset,
>                                                          the lock is released 
> */
> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
> index 4d123d9..652d9fc 100644
> --- a/include/hw/pci/pcie_regs.h
> +++ b/include/hw/pci/pcie_regs.h
> @@ -57,6 +57,8 @@
>  #define PCI_EXP_SLTCTL_PIC_SHIFT        (ffs(PCI_EXP_SLTCTL_PIC) - 1)
>  #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 |      \
> -- 
> 1.8.3.1