[v3,3/6] spapr_pci: encode class code including Prog IF register

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>
---
 hw/ppc/spapr_pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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.

> ---
>  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)));

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>