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.
ATB,
Mark.
