| Message ID | 1510926167-23326-12-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | sun4u: tidy-up CPU, APB and ebus | expand |
On Fri, Nov 17, 2017 at 2:42 PM, Mark Cave-Ayland <mark.cave-ayland@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-ayland@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. > cmd |= PCI_COMMAND_IO; > } > > @@ -673,14 +668,13 @@ static void pci_pbm_realize(DeviceState *dev, Error **errp) > pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 0), true, > TYPE_PBM_PCI_BRIDGE); > s->bridgeB = PCI_BRIDGE(pci_dev); > - pci_bridge_map_irq(s->bridgeB, "pciB", pci_pbm_map_irq); > + pci_bridge_map_irq(s->bridgeB, "pciB", pci_pbmB_map_irq); > qdev_init_nofail(&pci_dev->qdev); > > pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 1), true, > TYPE_PBM_PCI_BRIDGE); > s->bridgeA = PCI_BRIDGE(pci_dev); > - pci_bridge_map_irq(s->bridgeA, "pciA", pci_pbm_map_irq); > - qdev_prop_set_bit(DEVICE(pci_dev), "busA", true); > + pci_bridge_map_irq(s->bridgeA, "pciA", pci_pbmA_map_irq); > qdev_init_nofail(&pci_dev->qdev); > } > > @@ -789,11 +783,6 @@ static const TypeInfo pbm_host_info = { > .class_init = pbm_host_class_init, > }; > > -static Property pbm_pci_properties[] = { > - DEFINE_PROP_BOOL("busA", PBMPCIBridge, busA, false), > - DEFINE_PROP_END_OF_LIST(), > -}; > - > static void pbm_pci_bridge_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -809,7 +798,6 @@ static void pbm_pci_bridge_class_init(ObjectClass *klass, void *data) > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > dc->reset = pci_bridge_reset; > dc->vmsd = &vmstate_pci_device; > - dc->props = pbm_pci_properties; > } > > static const TypeInfo pbm_pci_bridge_info = { > diff --git a/include/hw/pci-host/apb.h b/include/hw/pci-host/apb.h > index f0074f7..dd49437 100644 > --- a/include/hw/pci-host/apb.h > +++ b/include/hw/pci-host/apb.h > @@ -86,9 +86,6 @@ typedef struct APBState { > typedef struct PBMPCIBridge { > /*< private >*/ > PCIBridge parent_obj; > - > - /* Is this busA with in-built devices (ebus)? */ > - bool busA; > } PBMPCIBridge; > > #define TYPE_PBM_PCI_BRIDGE "pbm-bridge" > -- > 1.7.10.4 >
On 17/11/17 14:33, Artyom Tarasenko wrote: > On Fri, Nov 17, 2017 at 2:42 PM, Mark Cave-Ayland > <mark.cave-ayland@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-ayland@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.
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-ayland@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-ayland@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? ATB, Mark.
On Sun, Dec 17, 2017 at 12:09 PM, Mark Cave-Ayland <mark.cave-ayland@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-ayland@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-ayland@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?
On 19/12/17 07:56, Artyom Tarasenko wrote: > On Sun, Dec 17, 2017 at 12:09 PM, Mark Cave-Ayland > <mark.cave-ayland@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-ayland@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-ayland@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? Yes, we could enable the PCI IO transaction bit for that particular bridge after realize if that is acceptable? ATB, Mark.
On Tue, Dec 19, 2017 at 10:27 AM, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 19/12/17 07:56, Artyom Tarasenko wrote: >> >> On Sun, Dec 17, 2017 at 12:09 PM, Mark Cave-Ayland >> <mark.cave-ayland@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-ayland@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-ayland@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? > > > Yes, we could enable the PCI IO transaction bit for that particular bridge > after realize if that is acceptable? Yes, please. I think it would be pretty close to what the real hw does.
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)) { cmd |= PCI_COMMAND_IO; } @@ -673,14 +668,13 @@ static void pci_pbm_realize(DeviceState *dev, Error **errp) pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 0), true, TYPE_PBM_PCI_BRIDGE); s->bridgeB = PCI_BRIDGE(pci_dev); - pci_bridge_map_irq(s->bridgeB, "pciB", pci_pbm_map_irq); + pci_bridge_map_irq(s->bridgeB, "pciB", pci_pbmB_map_irq); qdev_init_nofail(&pci_dev->qdev); pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 1), true, TYPE_PBM_PCI_BRIDGE); s->bridgeA = PCI_BRIDGE(pci_dev); - pci_bridge_map_irq(s->bridgeA, "pciA", pci_pbm_map_irq); - qdev_prop_set_bit(DEVICE(pci_dev), "busA", true); + pci_bridge_map_irq(s->bridgeA, "pciA", pci_pbmA_map_irq); qdev_init_nofail(&pci_dev->qdev); } @@ -789,11 +783,6 @@ static const TypeInfo pbm_host_info = { .class_init = pbm_host_class_init, }; -static Property pbm_pci_properties[] = { - DEFINE_PROP_BOOL("busA", PBMPCIBridge, busA, false), - DEFINE_PROP_END_OF_LIST(), -}; - static void pbm_pci_bridge_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -809,7 +798,6 @@ static void pbm_pci_bridge_class_init(ObjectClass *klass, void *data) set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); dc->reset = pci_bridge_reset; dc->vmsd = &vmstate_pci_device; - dc->props = pbm_pci_properties; } static const TypeInfo pbm_pci_bridge_info = { diff --git a/include/hw/pci-host/apb.h b/include/hw/pci-host/apb.h index f0074f7..dd49437 100644 --- a/include/hw/pci-host/apb.h +++ b/include/hw/pci-host/apb.h @@ -86,9 +86,6 @@ typedef struct APBState { typedef struct PBMPCIBridge { /*< private >*/ PCIBridge parent_obj; - - /* Is this busA with in-built devices (ebus)? */ - bool busA; } PBMPCIBridge; #define TYPE_PBM_PCI_BRIDGE "pbm-bridge"
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-ayland@ilande.co.uk> --- hw/pci-host/apb.c | 54 ++++++++++++++++++--------------------------- include/hw/pci-host/apb.h | 3 --- 2 files changed, 21 insertions(+), 36 deletions(-)