Re: [Qemu-devel] [PATCH v3 3/6] spapr_pci: encode class code including Prog IF register

[Thomas Huth <thuth@redhat.com>]

On Tue, 5 May 2015 22:55:00 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <nikunj@linux.vnet.ibm.com>
> 
> 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 <thuth@redhat.com>