From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47375) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WyfJA-000861-B6 for qemu-devel@nongnu.org; Sun, 22 Jun 2014 06:47:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WyfJ4-0003Ch-5y for qemu-devel@nongnu.org; Sun, 22 Jun 2014 06:47:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14260) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WyfJ3-0003CL-UP for qemu-devel@nongnu.org; Sun, 22 Jun 2014 06:47:30 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s5MAlSkM024368 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Sun, 22 Jun 2014 06:47:28 -0400 Message-ID: <1403434044.2109.3.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Sun, 22 Jun 2014 13:47:24 +0300 In-Reply-To: <20140619143915.GA10359@redhat.com> References: <1403185941-19561-1-git-send-email-marcel.a@redhat.com> <1403185941-19561-3-git-send-email-marcel.a@redhat.com> <20140619143915.GA10359@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org On Thu, 2014-06-19 at 17:39 +0300, Michael S. Tsirkin wrote: > 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 > > --- > > 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? Does Paolo's explanation answer your question? > > > > @@ -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? Yes > A switch can have multiple downstream ports at any slot #. It doesn't matter how many devices are under this slot, we need only one to power up the slot. The question here was "Do we have at least one device attahc to slot? If yes, power up the slot." > > > + int pic; > > uint16_t please. Sure, > > > + > > + 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. Sure, > > > + } > > + > > Need to disable for compat types? As above, Thanks for the review, Marcel > > > 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