diff mbox series

[4/8] hw/pci-host/designware: Hoist host controller in root function #0

Message ID 20231012121857.31873-5-philmd@linaro.org
State New
Headers show
Series hw/pci-host/designware: QOM shuffling (Host bridge <-> Root function) | expand

Commit Message

Philippe Mathieu-Daudé Oct. 12, 2023, 12:18 p.m. UTC 
There is always an unique root function for the host bridge
controller. We create this function when the controller is
realized, in designware_pcie_host_realize().

No need to call qdev_get_parent_bus() each time the root function
want to resolve its host part. Hoist a pointer in its state. Set
the pointer once when the function is realized.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci-host/designware.h |  1 +
 hw/pci-host/designware.c         | 15 +++++----------
 2 files changed, 6 insertions(+), 10 deletions(-)

Comments

Gustavo Romero Aug. 19, 2024, 4:22 a.m. UTC | #1 
Hi Phil,

I think the title of this patch is bit misleading. You're not
moving host into root, but rather adding a reference of host
inside the root.

Maybe change it to something like:

  "Add a back-pointer to the host in the root"?


On 10/12/23 9:18 AM, Philippe Mathieu-Daudé wrote:
> There is always an unique root function for the host bridge
> controller. We create this function when the controller is
> realized, in designware_pcie_host_realize().
> 
> No need to call qdev_get_parent_bus() each time the root function
> want to resolve its host part. Hoist a pointer in its state. Set
> the pointer once when the function is realized.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/pci-host/designware.h |  1 +
>   hw/pci-host/designware.c         | 15 +++++----------
>   2 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
> index c484e377a8..9e2caa04e9 100644
> --- a/include/hw/pci-host/designware.h
> +++ b/include/hw/pci-host/designware.h
> @@ -71,6 +71,7 @@ struct DesignwarePCIERoot {
>   
>       DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
>       DesignwarePCIEMSI msi;
> +    DesignwarePCIEHost *host;

Because root is still defined in host and you're actually adding a
reference of host inside root this is a back-pointer to host, so do
you mind adding a comment above this like saying something like:

/* Convenient back-pointer for easy access to the host interface. */

Otherwise:

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo


>   };
>   
>   struct DesignwarePCIEHost {
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index bacb2bdb2d..fb46493a05 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -57,13 +57,6 @@
>   
>   #define DESIGNWARE_PCIE_IRQ_MSI                    3
>   
> -static DesignwarePCIEHost *
> -designware_pcie_root_to_host(DesignwarePCIERoot *root)
> -{
> -    BusState *bus = qdev_get_parent_bus(DEVICE(root));
> -    return DESIGNWARE_PCIE_HOST(bus->parent);
> -}
> -
>   static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
>                                                 unsigned size)
>   {
> @@ -85,7 +78,7 @@ static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
>                                              uint64_t val, unsigned len)
>   {
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(opaque);
> -    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
> +    DesignwarePCIEHost *host = root->host;
>   
>       root->msi.intr[0].status |= BIT(val) & root->msi.intr[0].enable;
>   
> @@ -300,7 +293,7 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>                                                 uint32_t val, int len)
>   {
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
> -    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
> +    DesignwarePCIEHost *host = root->host;
>       DesignwarePCIEViewport *viewport =
>           designware_pcie_root_get_current_viewport(root);
>   
> @@ -392,7 +385,8 @@ static char *designware_pcie_viewport_name(const char *direction,
>   static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>   {
>       DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
> -    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
> +    DesignwarePCIEHost *host = DESIGNWARE_PCIE_HOST(
> +                                    qdev_get_parent_bus(DEVICE(dev))->parent);
>       MemoryRegion *host_mem = get_system_memory();
>       MemoryRegion *address_space = &host->pci.memory;
>       PCIBridge *br = PCI_BRIDGE(dev);
> @@ -406,6 +400,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>       size_t i;
>   
>       br->bus_name  = "dw-pcie";
> +    root->host = host;
>   
>       pci_set_word(dev->config + PCI_COMMAND,
>                    PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
>
diff mbox series

Patch

diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index c484e377a8..9e2caa04e9 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -71,6 +71,7 @@  struct DesignwarePCIERoot {
 
     DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
     DesignwarePCIEMSI msi;
+    DesignwarePCIEHost *host;
 };
 
 struct DesignwarePCIEHost {
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index bacb2bdb2d..fb46493a05 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -57,13 +57,6 @@ 
 
 #define DESIGNWARE_PCIE_IRQ_MSI                    3
 
-static DesignwarePCIEHost *
-designware_pcie_root_to_host(DesignwarePCIERoot *root)
-{
-    BusState *bus = qdev_get_parent_bus(DEVICE(root));
-    return DESIGNWARE_PCIE_HOST(bus->parent);
-}
-
 static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
                                               unsigned size)
 {
@@ -85,7 +78,7 @@  static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
                                            uint64_t val, unsigned len)
 {
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(opaque);
-    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
+    DesignwarePCIEHost *host = root->host;
 
     root->msi.intr[0].status |= BIT(val) & root->msi.intr[0].enable;
 
@@ -300,7 +293,7 @@  static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
                                               uint32_t val, int len)
 {
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(d);
-    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
+    DesignwarePCIEHost *host = root->host;
     DesignwarePCIEViewport *viewport =
         designware_pcie_root_get_current_viewport(root);
 
@@ -392,7 +385,8 @@  static char *designware_pcie_viewport_name(const char *direction,
 static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
 {
     DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
-    DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
+    DesignwarePCIEHost *host = DESIGNWARE_PCIE_HOST(
+                                    qdev_get_parent_bus(DEVICE(dev))->parent);
     MemoryRegion *host_mem = get_system_memory();
     MemoryRegion *address_space = &host->pci.memory;
     PCIBridge *br = PCI_BRIDGE(dev);
@@ -406,6 +400,7 @@  static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
     size_t i;
 
     br->bus_name  = "dw-pcie";
+    root->host = host;
 
     pci_set_word(dev->config + PCI_COMMAND,
                  PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);