On Sun, Dec 17, 2017 at 12:09 PM, Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> wrote: > On 19/11/17 11:06, Mark Cave-Ayland wrote: > >> On 17/11/17 14:33, Artyom Tarasenko wrote: >> >>> On Fri, Nov 17, 2017 at 2:42 PM, Mark Cave-Ayland >>> <mark.cave-ayl...@ilande.co.uk> wrote: >>>> >>>> After the previous refactoring it is now possible to use separate >>>> functions >>>> to improve clarity of the interrupt paths. Similarly by checking the PCI >>>> devnfn to identify busA during apb_pci_bridge_realize() it becomes >>>> possible >>>> to completely remove the busA property from the PBMPCIBridge state. >>>> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> >>>> --- >>>> hw/pci-host/apb.c | 54 >>>> ++++++++++++++++++--------------------------- >>>> include/hw/pci-host/apb.h | 3 --- >>>> 2 files changed, 21 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c >>>> index 6c20285..268100e 100644 >>>> --- a/hw/pci-host/apb.c >>>> +++ b/hw/pci-host/apb.c >>>> @@ -517,32 +517,27 @@ static int pci_apb_map_irq(PCIDevice *pci_dev, int >>>> irq_num) >>>> return irq_num; >>>> } >>>> >>>> -static int pci_pbm_map_irq(PCIDevice *pci_dev, int irq_num) >>>> +static int pci_pbmA_map_irq(PCIDevice *pci_dev, int irq_num) >>>> { >>>> - PBMPCIBridge *br = PBM_PCI_BRIDGE(pci_bridge_get_device( >>>> - >>>> PCI_BUS(qdev_get_parent_bus(DEVICE(pci_dev))))); >>>> - >>>> - int bus_offset; >>>> - if (br->busA) { >>>> - bus_offset = 0x0; >>>> + /* The on-board devices have fixed (legacy) OBIO intnos */ >>>> + switch (PCI_SLOT(pci_dev->devfn)) { >>>> + case 1: >>>> + /* Onboard NIC */ >>>> + return 0x21; >>>> + case 3: >>>> + /* Onboard IDE */ >>>> + return 0x20; >>>> + default: >>>> + /* Normal intno, fall through */ >>>> + break; >>>> + } >>>> >>>> - /* The on-board devices have fixed (legacy) OBIO intnos */ >>>> - switch (PCI_SLOT(pci_dev->devfn)) { >>>> - case 1: >>>> - /* Onboard NIC */ >>>> - return 0x21; >>>> - case 3: >>>> - /* Onboard IDE */ >>>> - return 0x20; >>>> + return ((PCI_SLOT(pci_dev->devfn) << 2) + irq_num) & 0x1f; >>>> +} >>>> >>>> - default: >>>> - /* Normal intno, fall through */ >>>> - break; >>>> - } >>>> - } else { >>>> - bus_offset = 0x10; >>>> - } >>>> - return (bus_offset + (PCI_SLOT(pci_dev->devfn) << 2) + irq_num) & >>>> 0x1f; >>>> +static int pci_pbmB_map_irq(PCIDevice *pci_dev, int irq_num) >>>> +{ >>>> + return (0x10 + (PCI_SLOT(pci_dev->devfn) << 2) + irq_num) & 0x1f; >>>> } >>>> >>>> static void pci_apb_set_irq(void *opaque, int irq_num, int level) >>>> @@ -593,7 +588,7 @@ static void apb_pci_bridge_realize(PCIDevice *dev, >>>> Error **errp) >>>> >>>> /* If initialising busA, ensure that we allow IO transactions so >>>> that >>>> we get the early serial console until OpenBIOS configures the >>>> bridge */ >>>> - if (br->busA) { >>>> + if (dev->devfn == PCI_DEVFN(1, 1)) { >>> >>> >>> I think the previous syntax was more explicit here. A comment would be >>> nice. >> >> >> Yes it's definitely something that isn't immediately obvious, which is >> why I left the above comment in place explaining what the if() branch is >> doing. Is there something in the comment that isn't particularly clear? >> >> Note one of the reasons for wanting to remove the busA property is that >> where possible I'd like to reduce the code in the IRQ path, and while >> the existing code works I am still unsure of the additional overhead of >> the 2 levels of QOM type checking that the current approach requires for >> each IRQ. > > > Hi Artyom, > > Thinking about this a bit more during freeze, this is actually doing the > opposite of what we want, as it requires the device realise function to > behave differently depending upon how it is related to the PCI bus. > > How about swapping this out for a qdev bool property for APB named > "enable-early-pci-io-access", setting it just for the PCI_DEVFN(1, 1) device > containing the ebus and then alter the if() statement above to enable PCI IO > access if the qdev property is set?
This does sound reasonable. I wonder if this has to be a qdev property though. Doesn't the physical bridge have a software visible bit/register for it? -- Regards, Artyom Tarasenko SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu