[Qemu-devel] [PATCH] apb_pci: fix header type of pbm pci host bridge.

[Qemu-devel] [PATCH] apb_pci: fix header type of pbm pci host bridge.

[Isaku Yamahata]

The change set of 776e1bbb6cf4fe66a93c1a5dd814bbb650deca00
specifies pbm pci host bridge is type of bridge.
It contradicts with pbm_pci_host_init().

Blue Swirl, could you please check this patch?
To be honest I don't know about pbm pci host bridge so that
I don't know which is correct, pbm_pci_host_init() or pbm_pci_host_info.
I just took the older code.

Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/apb_pci.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 46d5b0e..359a84f 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -471,7 +471,7 @@ static int pbm_pci_host_init(PCIDevice *d)
     d->config[0x09] = 0x00; // programming i/f
     pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
     d->config[0x0D] = 0x10; // latency_timer
-    d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
+    /* header type is initialized by do_pci_register_device() */
     return 0;
 }
 
@@ -479,7 +479,6 @@ static PCIDeviceInfo pbm_pci_host_info = {
     .qdev.name = "pbm",
     .qdev.size = sizeof(PCIDevice),
     .init      = pbm_pci_host_init,
-    .header_type  = PCI_HEADER_TYPE_BRIDGE,
 };
 
 static SysBusDeviceInfo pbm_host_info = {
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH] apb_pci: fix header type of pbm pci host bridge.

[Michael S. Tsirkin]

On Mon, Feb 08, 2010 at 03:45:03PM +0900, Isaku Yamahata wrote:
> The change set of 776e1bbb6cf4fe66a93c1a5dd814bbb650deca00
> specifies pbm pci host bridge is type of bridge.
> It contradicts with pbm_pci_host_init().
> 
> Blue Swirl, could you please check this patch?
> To be honest I don't know about pbm pci host bridge so that
> I don't know which is correct, pbm_pci_host_init() or pbm_pci_host_info.
> I just took the older code.
> 
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Blue Swirl, can you Ack please?

> ---
>  hw/apb_pci.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 46d5b0e..359a84f 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -471,7 +471,7 @@ static int pbm_pci_host_init(PCIDevice *d)
>      d->config[0x09] = 0x00; // programming i/f
>      pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
>      d->config[0x0D] = 0x10; // latency_timer

Looks like apb_pci needs another sweep of getting rid of
hard-coded constants. Any takers?

> -    d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
> +    /* header type is initialized by do_pci_register_device() */

Let's not put such comments around code: that function can get renamed
or removed or moved to another file and no one will remember to update
this comment. This belongs in commit message.

>      return 0;
>  }
>  
> @@ -479,7 +479,6 @@ static PCIDeviceInfo pbm_pci_host_info = {
>      .qdev.name = "pbm",
>      .qdev.size = sizeof(PCIDevice),
>      .init      = pbm_pci_host_init,
> -    .header_type  = PCI_HEADER_TYPE_BRIDGE,
>  };
>  
>  static SysBusDeviceInfo pbm_host_info = {
> -- 
> 1.6.6.1

[Qemu-devel] Re: [PATCH] apb_pci: fix header type of pbm pci host bridge.

[Michael S. Tsirkin]

On Mon, Feb 08, 2010 at 03:45:03PM +0900, Isaku Yamahata wrote:
> The change set of 776e1bbb6cf4fe66a93c1a5dd814bbb650deca00
> specifies pbm pci host bridge is type of bridge.
> It contradicts with pbm_pci_host_init().


By the way, the next below (cover letter) should be put after
--- rather than here, so that it does not end up in
commit message.


> Blue Swirl, could you please check this patch?
> To be honest I don't know about pbm pci host bridge so that
> I don't know which is correct, pbm_pci_host_init() or pbm_pci_host_info.
> I just took the older code.
> 
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/apb_pci.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 46d5b0e..359a84f 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -471,7 +471,7 @@ static int pbm_pci_host_init(PCIDevice *d)
>      d->config[0x09] = 0x00; // programming i/f
>      pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
>      d->config[0x0D] = 0x10; // latency_timer
> -    d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
> +    /* header type is initialized by do_pci_register_device() */
>      return 0;
>  }
>  
> @@ -479,7 +479,6 @@ static PCIDeviceInfo pbm_pci_host_info = {
>      .qdev.name = "pbm",
>      .qdev.size = sizeof(PCIDevice),
>      .init      = pbm_pci_host_init,
> -    .header_type  = PCI_HEADER_TYPE_BRIDGE,
>  };
>  
>  static SysBusDeviceInfo pbm_host_info = {
> -- 
> 1.6.6.1

[Qemu-devel] Re: [PATCH] apb_pci: fix header type of pbm pci host bridge.

[Blue Swirl]

On Mon, Feb 8, 2010 at 8:45 AM, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> The change set of 776e1bbb6cf4fe66a93c1a5dd814bbb650deca00
> specifies pbm pci host bridge is type of bridge.
> It contradicts with pbm_pci_host_init().

Bridge header type in qdev info is needed so that the write masks are
correct. Otherwise the masks make for example PCI_SECONDARY_BUS
readonly. But the device uses PCI_HEADER_TYPE_NORMAL. So both are
correct.