From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47099) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpeBo-0004Ps-CD for qemu-devel@nongnu.org; Tue, 05 May 2015 10:51:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YpeBj-0002gl-TQ for qemu-devel@nongnu.org; Tue, 05 May 2015 10:51:16 -0400 Date: Tue, 5 May 2015 16:50:58 +0200 From: Thomas Huth Message-ID: <20150505165058.6d1256ce@thh440s> In-Reply-To: <20150505125500.GU14090@voom.redhat.com> References: <1430816036-26408-1-git-send-email-nikunj@linux.vnet.ibm.com> <1430816036-26408-4-git-send-email-nikunj@linux.vnet.ibm.com> <20150505125500.GU14090@voom.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/6] spapr_pci: encode class code including Prog IF register List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org, Nikunj A Dadhania , aik@ozlabs.ru, agraf@suse.de, mdroth@linux.vnet.ibm.com, qemu-ppc@nongnu.org On Tue, 5 May 2015 22:55:00 +1000 David Gibson wrote: > On Tue, May 05, 2015 at 02:23:53PM +0530, Nikunj A Dadhania wrote: > > Current code missed the Prog IF register. All Class Code, Subclass, > > and Prog IF registers are needed to identify the accurate device type. > > > > For example: USB controllers use the PROG IF for denoting: USB > > FullSpeed, HighSpeed or SuperSpeed. > > > > Signed-off-by: Nikunj A Dadhania > > Um.. I'm guessing the CLASS_PROG register essentially includes the > CLASS_DEVICE value? Otherwise it looks like you're losing the > CLASS_DEVICE value. > > For the benefit of those who don't remember the PCI spec from memory, > can you explain in more detail what the situation is with the several > class registers and how they overlap / interact. In the PCI local bus spec, the "Class Code" is a 3-bytes field starting at offset 9 in the config space. The QEMU defines seem to split this up into PCI_CLASS_PROG (9) and PCI_CLASS_DEVICE (10), but originally they belong together. > > --- > > hw/ppc/spapr_pci.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index ea1a092..8b02a3e 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -899,8 +899,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, > > _FDT(fdt_setprop_cell(fdt, offset, "revision-id", > > pci_default_read_config(dev, PCI_REVISION_ID, 1))); > > _FDT(fdt_setprop_cell(fdt, offset, "class-code", > > - pci_default_read_config(dev, PCI_CLASS_DEVICE, 2) > > - << 8)); > > + pci_default_read_config(dev, PCI_CLASS_PROG, 3))); > > if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) { > > _FDT(fdt_setprop_cell(fdt, offset, "interrupts", > > pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1))); > Patch looks fine to me. Reviewed-by: Thomas Huth