[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT |
Date: |
Tue, 22 Dec 2015 11:34:46 +0200 |
On Mon, Dec 21, 2015 at 01:55:16PM +0100, Igor Mammedov wrote:
> On Sat, 19 Dec 2015 21:23:22 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
>
> > On Thu, Dec 10, 2015 at 05:17:07PM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > > v2:
> > > - adapt build_prt() for using for PCI0._PRT(), reduces code duplication,
> > > Suggested-by: Marcel Apfelbaum <address@hidden>
> > >
> > > pc: acpi: piix4: adapt build_prt() for using for PCI0._PRT()
> > >
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > > hw/i386/acpi-build.c | 41 +++++++++++++++++++++++++++--------
> > > hw/i386/acpi-dsdt.dsl | 60
> > > ---------------------------------------------------
> > > 2 files changed, 32 insertions(+), 69 deletions(-)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index cf98037..1b065df 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -620,6 +620,17 @@ static void build_append_pci_bus_devices(Aml
> > > *parent_scope, PCIBus *bus,
> > > qobject_decref(bsel);
> > > }
> > >
> > > +static Aml *build_prt_entry(const char *link_name)
Pls document what this does.
> > > +{
> > > + Aml *a_zero = aml_int(0);
> > > + Aml *pkg = aml_package(4);
> > > + aml_append(pkg, a_zero);
> > > + aml_append(pkg, a_zero);
> > > + aml_append(pkg, aml_name("%s", link_name));
> > > + aml_append(pkg, a_zero);
> > > + return pkg;
> > > +}
> > > +
> > > /*
> > > * initialize_route - Initialize the interrupt routing rule
> > > * through a specific LINK:
> > > @@ -630,12 +641,8 @@ static Aml *initialize_route(Aml *route, const char
> > > *link_name,
> > > Aml *lnk_idx, int idx)
> > > {
> > > Aml *if_ctx = aml_if(aml_equal(lnk_idx, aml_int(idx)));
> > > - Aml *pkg = aml_package(4);
> > > + Aml *pkg = build_prt_entry(link_name);
> > >
> > > - aml_append(pkg, aml_int(0));
> > > - aml_append(pkg, aml_int(0));
> > > - aml_append(pkg, aml_name("%s", link_name));
> > > - aml_append(pkg, aml_int(0));
> > > aml_append(if_ctx, aml_store(pkg, route));
> > >
> > > return if_ctx;
> > > @@ -651,9 +658,9 @@ static Aml *initialize_route(Aml *route, const char
> > > *link_name,
> > > * The hash function is (slot + pin) & 3 -> "LNK[D|A|B|C]".
> > > *
> > > */
> > > -static Aml *build_prt(void)
> > > +static Aml *build_prt(bool is_pci0_prt)
> > > {
> > > - Aml *method, *while_ctx, *pin, *res;
> > > + Aml *method, *while_ctx, *pin, *res, *if_ctx, *if_ctx2, *else_ctx2;
> >
> > Move if_ctx2/if_ctx/else_ctx2 where they are used?
> > Also, can't we come up with better name than if_ctx2?
> > How about if_lnk_1 and if_pin_4?
> sure
>
> >
> > >
> > > method = aml_method("_PRT", 0, AML_NOTSERIALIZED);
> > > res = aml_local(0);
> > > @@ -678,7 +685,19 @@ static Aml *build_prt(void)
> > >
> > > /* route[2] = "LNK[D|A|B|C]", selection based on pin % 3 */
> > > aml_append(while_ctx, initialize_route(route, "LNKD", lnk_idx,
> > > 0));
> > > - aml_append(while_ctx, initialize_route(route, "LNKA", lnk_idx,
> > > 1));
> > > + if (is_pci0_prt) {
> > > + if_ctx = aml_if(aml_equal(lnk_idx, aml_int(1)));
> > > + /* device 1 is the power-management device, needs SCI */
> >
> > Doesn't this context belong above the previous line?
> that how it was in ASL, but yep it belongs to a whole if block
> so I'll move it up
>
> >
> > > + if_ctx2 = aml_if(aml_equal(pin, aml_int(4)));
> > > + aml_append(if_ctx2, aml_store(build_prt_entry("LNKS"),
> > > route));
> > > + aml_append(if_ctx, if_ctx2);
> > > + else_ctx2 = aml_else();
> > > + aml_append(else_ctx2, aml_store(build_prt_entry("LNKA"),
> > > route));
> > > + aml_append(if_ctx, else_ctx2);
> >
> > This looks weird. Why isn't else_ctx2 added to if_ctx2?
> it should be if_ctx2, I'll fix it.
Hmm I'm not sure actually.
The API for if/else is confusing, but I'm not sure what's
a better one.
> >
> > > + aml_append(while_ctx, if_ctx);
> > > + } else {
> > > + aml_append(while_ctx, initialize_route(route, "LNKA",
> > > lnk_idx, 1));
> > > + }
> > > aml_append(while_ctx, initialize_route(route, "LNKB", lnk_idx,
> > > 2));
> > > aml_append(while_ctx, initialize_route(route, "LNKC", lnk_idx,
> > > 3));
> > >
> > > @@ -1348,6 +1367,10 @@ static void build_piix4_pci0_int(Aml *table)
> > > Aml *method;
> > > uint32_t irqs;
> > > Aml *sb_scope = aml_scope("_SB");
> > > + Aml *pci0_scope = aml_scope("PCI0");
> > > +
> > > + aml_append(pci0_scope, build_prt(true));
> > > + aml_append(sb_scope, pci0_scope);
> > >
> > > field = aml_field("PCI0.ISA.P40C", AML_BYTE_ACC, AML_NOLOCK,
> > > AML_PRESERVE);
> > > aml_append(field, aml_named_field("PRQ0", 8));
> > > @@ -1569,7 +1592,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > aml_append(dev, aml_name_decl("_PXM",
> > > aml_int(numa_node)));
> > > }
> > >
> > > - aml_append(dev, build_prt());
> > > + aml_append(dev, build_prt(false));
> > > crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent),
> > > io_ranges, mem_ranges);
> > > aml_append(dev, aml_name_decl("_CRS", crs));
> > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > > index bc6bd45..5d741dd 100644
> > > --- a/hw/i386/acpi-dsdt.dsl
> > > +++ b/hw/i386/acpi-dsdt.dsl
> > > @@ -78,64 +78,4 @@ DefinitionBlock (
> > > /* Hotplug notification method supplied by SSDT */
> > > External(\_SB.PCI0.PCNT, MethodObj)
> > > }
> > > -
> > > -
> > > -/****************************************************************
> > > - * PCI IRQs
> > > - ****************************************************************/
> > > -
> > > - Scope(\_SB) {
> > > - Scope(PCI0) {
> > > - Method (_PRT, 0) {
> > > - Store(Package(128) {}, Local0)
> > > - Store(Zero, Local1)
> > > - While(LLess(Local1, 128)) {
> > > - // slot = pin >> 2
> > > - Store(ShiftRight(Local1, 2), Local2)
> > > -
> > > - // lnk = (slot + pin) & 3
> > > - Store(And(Add(Local1, Local2), 3), Local3)
> > > - If (LEqual(Local3, 0)) {
> > > - Store(Package(4) { Zero, Zero, LNKD, Zero },
> > > Local4)
> > > - }
> > > - If (LEqual(Local3, 1)) {
> > > - // device 1 is the power-management device,
> > > needs SCI
> > > - If (LEqual(Local1, 4)) {
> > > - Store(Package(4) { Zero, Zero, LNKS, Zero },
> > > Local4)
> > > - } Else {
> > > - Store(Package(4) { Zero, Zero, LNKA, Zero },
> > > Local4)
> > > - }
> > > - }
> > > - If (LEqual(Local3, 2)) {
> > > - Store(Package(4) { Zero, Zero, LNKB, Zero },
> > > Local4)
> > > - }
> > > - If (LEqual(Local3, 3)) {
> > > - Store(Package(4) { Zero, Zero, LNKC, Zero },
> > > Local4)
> > > - }
> > > -
> > > - // Complete the interrupt routing entry:
> > > - // Package(4) { 0x[slot]FFFF, [pin], [link], 0) }
> > > -
> > > - Store(Or(ShiftLeft(Local2, 16), 0xFFFF),
> > > Index(Local4, 0))
> > > - Store(And(Local1, 3),
> > > Index(Local4, 1))
> > > - Store(Local4,
> > > Index(Local0, Local1))
> > > -
> > > - Increment(Local1)
> > > - }
> > > -
> > > - Return(Local0)
> >
> > Interesting. And where's this code? Probably in previous or follow-up
> > patches ...
> it was and still is in original build_prt(),
> I'm just modifying it to generate missing parts of DSDT AML
> equivalent which it's replacing.
OK so commit log should explain that PRT for expander
buses was already generated in C, the only difference for
root bus is LINKA.
And add comment explaining that for pci0,
device 1 is connected to SCI (LNKS)
(maybe rename flag to device_1_is_pm?).
> >
> > > - }
> > > - }
> > > -
> > > -
> > > - External(PRQ0, FieldUnitObj)
> > > - External(PRQ1, FieldUnitObj)
> > > - External(PRQ2, FieldUnitObj)
> > > - External(PRQ3, FieldUnitObj)
> > > - External(LNKA, DeviceObj)
> > > - External(LNKB, DeviceObj)
> > > - External(LNKC, DeviceObj)
> > > - External(LNKD, DeviceObj)
> > > - External(LNKS, DeviceObj)
> > > - }
> > > }
> > > --
> > > 1.8.3.1
> > >
- Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT, (continued)
[Qemu-devel] [PATCH 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Igor Mammedov, 2015/12/09
- Re: [Qemu-devel] [PATCH 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Marcel Apfelbaum, 2015/12/10
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Michael S. Tsirkin, 2015/12/19
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Igor Mammedov, 2015/12/21
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Igor Mammedov, 2015/12/22
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Michael S. Tsirkin, 2015/12/22
[Qemu-devel] [PATCH 69/74] pc: acpi: q35: move _PIC() method into SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 73/74] pc: acpi: switch to AML API composed DSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 46/74] pc: acpi: move DBUG() from DSDT to SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 56/74] pc: acpi: piix4: move IQCR() into SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 59/74] pc: acpi: piix4: move remaining PCI hotplug bits into SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 64/74] pc: acpi: q35: move IQST() into SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 62/74] pc: acpi: q35: move link devices to SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 68/74] pc: acpi: q35: move PRTP routing table into SSDT, Igor Mammedov, 2015/12/09