Change in coreboot[master]: soc/amd/picasso: Correctly populate the PCI interrupt line register
Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48665 )
Change subject: soc/amd/picasso: Correctly populate the PCI interrupt line register ......................................................................
soc/amd/picasso: Correctly populate the PCI interrupt line register
The PCI interrupt line registers are used as a last resort if routing can't be fetched from either ACPI or the MPTable. This change correctly sets the registers. It overrides the pirq_data set by the mainboards since the routing is fixed in AGESA.
BUG=b:170595019 TEST=Boot ezkinil with `pci=nomsi,noacpi amd_iommu=off noapic` Verified all PCI peripherals are still functional.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: If5d4d8f613c8d0fa9b43cefa804824681c3410d6 --- M src/soc/amd/picasso/fch.c A src/soc/amd/picasso/include/soc/pci.h M src/soc/amd/picasso/pcie_gpp.c 3 files changed, 57 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/48665/1
diff --git a/src/soc/amd/picasso/fch.c b/src/soc/amd/picasso/fch.c index cda509b..85e7788 100644 --- a/src/soc/amd/picasso/fch.c +++ b/src/soc/amd/picasso/fch.c @@ -22,6 +22,7 @@ #include <soc/southbridge.h> #include <soc/smi.h> #include <soc/amd_pci_int_defs.h> +#include <soc/pci.h> #include <soc/pci_devs.h> #include <soc/nvs.h> #include <types.h> @@ -251,6 +252,9 @@ /* Write PCI_INTR regs 0xC00/0xC01 */ write_pci_int_table();
+ /* pirq_data is consumed by `write_pci_cfg_irqs` */ + populate_pirq_data(); + /* Write IRQs for all devicetree enabled devices */ write_pci_cfg_irqs(); } diff --git a/src/soc/amd/picasso/include/soc/pci.h b/src/soc/amd/picasso/include/soc/pci.h new file mode 100644 index 0000000..b6ff349 --- /dev/null +++ b/src/soc/amd/picasso/include/soc/pci.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef AMD_PICASSO_PCI_H +#define AMD_PICASSO_PCI_H + +void populate_pirq_data(void); + +#endif /* AMD_PICASSO_PCI_H */ diff --git a/src/soc/amd/picasso/pcie_gpp.c b/src/soc/amd/picasso/pcie_gpp.c index 1b65118..ca368f9 100644 --- a/src/soc/amd/picasso/pcie_gpp.c +++ b/src/soc/amd/picasso/pcie_gpp.c @@ -2,10 +2,12 @@
#include <acpi/acpigen.h> #include <assert.h> +#include <amdblocks/amd_pci_util.h> #include <device/device.h> #include <device/pci.h> #include <device/pciexp.h> #include <device/pci_ids.h> +#include <soc/pci.h> #include <soc/pci_devs.h> #include <stdio.h>
@@ -34,6 +36,26 @@ {PCIE_GPP_B_DEVFN, 7, "CDAB"}, };
+/* + * This data structure is populated from the raw data above. It is used + * by amd/common/block/pci/amd_pci_util to write the PCI_INT_LINE register + * to each PCI device. + */ +static struct pirq_struct pirq_data[] = { + { PCIE_GPP_0_DEVFN }, + { PCIE_GPP_1_DEVFN }, + { PCIE_GPP_2_DEVFN }, + { PCIE_GPP_3_DEVFN }, + { PCIE_GPP_4_DEVFN }, + { PCIE_GPP_5_DEVFN }, + { PCIE_GPP_6_DEVFN }, + { PCIE_GPP_A_DEVFN }, + { PCIE_GPP_B_DEVFN }, +}; + +_Static_assert(ARRAY_SIZE(pci_routing_table) == ARRAY_SIZE(pirq_data), + "PCI and PIRQ tables must be the same size"); + static const struct pci_routing *get_pci_routing(unsigned int devfn) { for (size_t i = 0; i < ARRAY_SIZE(pci_routing_table); ++i) { @@ -53,6 +75,29 @@ return irq_index; }
+void populate_pirq_data(void) +{ + const struct pci_routing *pci_routing; + struct pirq_struct *pirq; + unsigned int irq_index; + + for (size_t i = 0; i < ARRAY_SIZE(pirq_data); ++i) { + pirq = &pirq_data[i]; + pci_routing = get_pci_routing(pirq->devfn); + if (!pci_routing) + die("%s: devfn %u not found\n", __func__, pirq->devfn); + + for (size_t j = 0; j < 4; ++j) { + irq_index = calculate_irq(pci_routing, j); + + pirq->PIN[j] = irq_index % 8; + } + } + + pirq_data_ptr = pirq_data; + pirq_data_size = ARRAY_SIZE(pirq_data); +} + static const char *pcie_gpp_acpi_name(const struct device *dev) { if (dev->path.type != DEVICE_PATH_PCI)
Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48665 )
Change subject: soc/amd/picasso: Correctly populate the PCI interrupt line register ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48665 )
Change subject: soc/amd/picasso: Correctly populate the PCI interrupt line register ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48665/1/src/soc/amd/picasso/pcie_gp... File src/soc/amd/picasso/pcie_gpp.c:
https://review.coreboot.org/c/coreboot/+/48665/1/src/soc/amd/picasso/pcie_gp... PS1, Line 44: static struct pirq_struct pirq_data[] = { I guess duplicating this table could be avoided by moving the PCI routing code to amd_pci_util.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48665 )
Change subject: soc/amd/picasso: Correctly populate the PCI interrupt line register ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48665/1/src/soc/amd/picasso/pcie_gp... File src/soc/amd/picasso/pcie_gpp.c:
https://review.coreboot.org/c/coreboot/+/48665/1/src/soc/amd/picasso/pcie_gp... PS1, Line 44: static struct pirq_struct pirq_data[] = {
I guess duplicating this table could be avoided by moving the PCI routing code to amd_pci_util.
If amd_pci_util took in the pci_routing table above then yeah, we could move this code. I'm not sure how the PCI routing works in other older SoCs though.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48665 )
Change subject: soc/amd/picasso: Correctly populate the PCI interrupt line register ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48665/1/src/soc/amd/picasso/pcie_gp... File src/soc/amd/picasso/pcie_gpp.c:
https://review.coreboot.org/c/coreboot/+/48665/1/src/soc/amd/picasso/pcie_gp... PS1, Line 44: static struct pirq_struct pirq_data[] = {
If amd_pci_util took in the pci_routing table above then yeah, we could move this code. […]
it has a bit of redundancy, but i'm ok with that for now. with the following 2 patches things do get less redundant and before the routing info was just wrong
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48665 )
Change subject: soc/amd/picasso: Correctly populate the PCI interrupt line register ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/48665/1/src/soc/amd/picasso/pcie_gp... File src/soc/amd/picasso/pcie_gpp.c:
https://review.coreboot.org/c/coreboot/+/48665/1/src/soc/amd/picasso/pcie_gp... PS1, Line 44: static struct pirq_struct pirq_data[] = {
it has a bit of redundancy, but i'm ok with that for now. […]
Yeah, I'm fine with this for now. I guess Cezanne also has some PCI IRQ and/or PIRQ routing to do, so I would expect this to be revisited in the near future anyway 😊
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48665 )
Change subject: soc/amd/picasso: Correctly populate the PCI interrupt line register ......................................................................
soc/amd/picasso: Correctly populate the PCI interrupt line register
The PCI interrupt line registers are used as a last resort if routing can't be fetched from either ACPI or the MPTable. This change correctly sets the registers. It overrides the pirq_data set by the mainboards since the routing is fixed in AGESA.
BUG=b:170595019 TEST=Boot ezkinil with `pci=nomsi,noacpi amd_iommu=off noapic` Verified all PCI peripherals are still functional.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: If5d4d8f613c8d0fa9b43cefa804824681c3410d6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/48665 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nikolai Vyssotski nikolai.vyssotski@amd.corp-partner.google.com Reviewed-by: Felix Held felix-coreboot@felixheld.de Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/soc/amd/picasso/fch.c A src/soc/amd/picasso/include/soc/pci.h M src/soc/amd/picasso/pcie_gpp.c 3 files changed, 57 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Nikolai Vyssotski: Looks good to me, but someone else must approve
diff --git a/src/soc/amd/picasso/fch.c b/src/soc/amd/picasso/fch.c index d5278cb..4c8c584 100644 --- a/src/soc/amd/picasso/fch.c +++ b/src/soc/amd/picasso/fch.c @@ -22,6 +22,7 @@ #include <soc/southbridge.h> #include <soc/smi.h> #include <soc/amd_pci_int_defs.h> +#include <soc/pci.h> #include <soc/pci_devs.h> #include <soc/nvs.h> #include <types.h> @@ -259,6 +260,9 @@ /* Write PCI_INTR regs 0xC00/0xC01 */ write_pci_int_table();
+ /* pirq_data is consumed by `write_pci_cfg_irqs` */ + populate_pirq_data(); + /* Write IRQs for all devicetree enabled devices */ write_pci_cfg_irqs(); } diff --git a/src/soc/amd/picasso/include/soc/pci.h b/src/soc/amd/picasso/include/soc/pci.h new file mode 100644 index 0000000..b6ff349 --- /dev/null +++ b/src/soc/amd/picasso/include/soc/pci.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef AMD_PICASSO_PCI_H +#define AMD_PICASSO_PCI_H + +void populate_pirq_data(void); + +#endif /* AMD_PICASSO_PCI_H */ diff --git a/src/soc/amd/picasso/pcie_gpp.c b/src/soc/amd/picasso/pcie_gpp.c index 1b65118..ca368f9 100644 --- a/src/soc/amd/picasso/pcie_gpp.c +++ b/src/soc/amd/picasso/pcie_gpp.c @@ -2,10 +2,12 @@
#include <acpi/acpigen.h> #include <assert.h> +#include <amdblocks/amd_pci_util.h> #include <device/device.h> #include <device/pci.h> #include <device/pciexp.h> #include <device/pci_ids.h> +#include <soc/pci.h> #include <soc/pci_devs.h> #include <stdio.h>
@@ -34,6 +36,26 @@ {PCIE_GPP_B_DEVFN, 7, "CDAB"}, };
+/* + * This data structure is populated from the raw data above. It is used + * by amd/common/block/pci/amd_pci_util to write the PCI_INT_LINE register + * to each PCI device. + */ +static struct pirq_struct pirq_data[] = { + { PCIE_GPP_0_DEVFN }, + { PCIE_GPP_1_DEVFN }, + { PCIE_GPP_2_DEVFN }, + { PCIE_GPP_3_DEVFN }, + { PCIE_GPP_4_DEVFN }, + { PCIE_GPP_5_DEVFN }, + { PCIE_GPP_6_DEVFN }, + { PCIE_GPP_A_DEVFN }, + { PCIE_GPP_B_DEVFN }, +}; + +_Static_assert(ARRAY_SIZE(pci_routing_table) == ARRAY_SIZE(pirq_data), + "PCI and PIRQ tables must be the same size"); + static const struct pci_routing *get_pci_routing(unsigned int devfn) { for (size_t i = 0; i < ARRAY_SIZE(pci_routing_table); ++i) { @@ -53,6 +75,29 @@ return irq_index; }
+void populate_pirq_data(void) +{ + const struct pci_routing *pci_routing; + struct pirq_struct *pirq; + unsigned int irq_index; + + for (size_t i = 0; i < ARRAY_SIZE(pirq_data); ++i) { + pirq = &pirq_data[i]; + pci_routing = get_pci_routing(pirq->devfn); + if (!pci_routing) + die("%s: devfn %u not found\n", __func__, pirq->devfn); + + for (size_t j = 0; j < 4; ++j) { + irq_index = calculate_irq(pci_routing, j); + + pirq->PIN[j] = irq_index % 8; + } + } + + pirq_data_ptr = pirq_data; + pirq_data_size = ARRAY_SIZE(pirq_data); +} + static const char *pcie_gpp_acpi_name(const struct device *dev) { if (dev->path.type != DEVICE_PATH_PCI)
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
Nikolai Vyssotski (Code Review)
-
Raul Rangel (Code Review)