coreboot-gerrit November 2019

coreboot-gerrit@coreboot.org

1 participants
2418 discussions

Change in coreboot[master]: mb/google/hatch/var/akemi: disable unused USB port for Akemi platform
by Peichao Li (Code Review) 08 Nov '19

08 Nov '19

Peichao Li has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36644 ) Change subject: mb/google/hatch/var/akemi: disable unused USB port for Akemi platform ...................................................................... mb/google/hatch/var/akemi: disable unused USB port for Akemi platform Akemi platform dosen't support WWAN device and unused USB2 port 4, 5, 7, 8 and USB3 port 4, 5 so close them. BUG=None TEST=FW_NAME="akemi" emerge-hatch coreboot chromeos-ec chromeos-bootimage Signed-off-by: Peichao.Wang <peichao.wang(a)bitland.corp-partner.google.com> Change-Id: I7eff4da77caea7d4fe46597320be134d34d78a22 --- M src/mainboard/google/hatch/variants/akemi/overridetree.cb 1 file changed, 109 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36644/1 diff --git a/src/mainboard/google/hatch/variants/akemi/overridetree.cb b/src/mainboard/google/hatch/variants/akemi/overridetree.cb index b58beaa..f69e8f6 100644 --- a/src/mainboard/google/hatch/variants/akemi/overridetree.cb +++ b/src/mainboard/google/hatch/variants/akemi/overridetree.cb @@ -15,6 +15,24 @@ [PchSerialIoIndexUART2] = PchSerialIoDisabled, }" + register "usb2_ports[0]" = "USB2_PORT_TYPE_C(OC2)" # Type-C Port 0 + register "usb2_ports[1]" = "USB2_PORT_TYPE_C(OC2)" # Type-C Port 1 + register "usb2_ports[2]" = "USB2_PORT_SHORT(OC3)" # Type-A Port 0 + register "usb2_ports[3]" = "USB2_PORT_LONG(OC3)" # Type-A Port 1 + register "usb2_ports[4]" = "USB2_PORT_EMPTY" + register "usb2_ports[5]" = "USB2_PORT_EMPTY" # WWAN + register "usb2_ports[6]" = "USB2_PORT_LONG(OC_SKIP)" # Camera + register "usb2_ports[7]" = "USB2_PORT_EMPTY" + register "usb2_ports[8]" = "USB2_PORT_EMPTY" + register "usb2_ports[9]" = "USB2_PORT_MID(OC_SKIP)" # BT + + register "usb3_ports[0]" = "USB3_PORT_DEFAULT(OC2)" # Type-C Port 0 + register "usb3_ports[1]" = "USB3_PORT_DEFAULT(OC2)" # Type-C Port 1 + register "usb3_ports[2]" = "USB3_PORT_DEFAULT(OC3)" # Type-A Port 0 + register "usb3_ports[3]" = "USB3_PORT_DEFAULT(OC3)" # Type-A Port 1 + register "usb3_ports[4]" = "USB3_PORT_EMPTY" # WWAN + register "usb3_ports[5]" = "USB3_PORT_EMPTY" + # Intel Common SoC Config #+-------------------+---------------------------+ #| Field | Value | @@ -62,6 +80,97 @@ register "ScsEmmcHs400Enabled" = "1" device domain 0 on + device pci 14.0 on + chip drivers/usb/acpi + register "desc" = ""Root Hub"" + register "type" = "UPC_TYPE_HUB" + device usb 0.0 on + chip drivers/usb/acpi + register "desc" = ""Left Type-C Port"" + register "type" = "UPC_TYPE_C_USB2_SS_SWITCH" + register "group" = "ACPI_PLD_GROUP(1, 1)" + device usb 2.0 on end + end + chip drivers/usb/acpi + register "desc" = ""Right Type-C Port 1"" + register "type" = "UPC_TYPE_C_USB2_SS_SWITCH" + register "group" = "ACPI_PLD_GROUP(2, 1)" + device usb 2.1 on end + end + chip drivers/usb/acpi + register "desc" = ""Left Type-A Port"" + register "type" = "UPC_TYPE_A" + register "group" = "ACPI_PLD_GROUP(1, 2)" + device usb 2.2 on end + end + chip drivers/usb/acpi + register "desc" = ""Right Type-A Port 1"" + register "type" = "UPC_TYPE_A" + register "group" = "ACPI_PLD_GROUP(2, 2)" + device usb 2.3 on end + end + chip drivers/usb/acpi + # Unused + device usb 2.4 off end + end + chip drivers/usb/acpi + # No WWAN + device usb 2.5 off end + end + chip drivers/usb/acpi + register "desc" = ""Camera"" + register "type" = "UPC_TYPE_INTERNAL" + device usb 2.6 on end + end + chip drivers/usb/acpi + # Unused + device usb 2.7 off end + end + chip drivers/usb/acpi + # Unused + device usb 2.8 off end + end + chip drivers/usb/acpi + register "desc" = ""Bluetooth"" + register "type" = "UPC_TYPE_INTERNAL" + register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_C14)" + device usb 2.9 on end + end + chip drivers/usb/acpi + register "desc" = ""Left Type-C Port"" + register "type" = "UPC_TYPE_C_USB2_SS_SWITCH" + register "group" = "ACPI_PLD_GROUP(1, 1)" + device usb 3.0 on end + end + chip drivers/usb/acpi + register "desc" = ""Right Type-C Port 1"" + register "type" = "UPC_TYPE_C_USB2_SS_SWITCH" + register "group" = "ACPI_PLD_GROUP(2, 1)" + device usb 3.1 on end + end + chip drivers/usb/acpi + register "desc" = ""Left Type-A Port"" + register "type" = "UPC_TYPE_USB3_A" + register "group" = "ACPI_PLD_GROUP(1, 2)" + device usb 3.2 on end + end + chip drivers/usb/acpi + register "desc" = ""Right Type-A Port 1"" + register "type" = "UPC_TYPE_USB3_A" + register "group" = "ACPI_PLD_GROUP(2, 2)" + device usb 3.3 on end + end + chip drivers/usb/acpi + # No WWAN + device usb 3.4 off end + end + chip drivers/usb/acpi + # Unused + device usb 3.5 off end + end + end + end + end # USB xHCI device pci 15.0 on chip drivers/i2c/generic register "hid" = ""ELAN0000"" -- To view, visit https://review.coreboot.org/c/coreboot/+/36644 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7eff4da77caea7d4fe46597320be134d34d78a22 Gerrit-Change-Number: 36644 Gerrit-PatchSet: 1 Gerrit-Owner: Peichao Li <peichao.wang(a)bitland.corp-partner.google.com> Gerrit-MessageType: newchange

4 6

Change in coreboot[master]: include: introduce update* for mmio operations
by Aaron Durbin (Code Review) 07 Nov '19

07 Nov '19

1 0

Change in coreboot[master]: include: introduce update* for mmio operations
by Aaron Durbin (Code Review) 07 Nov '19

07 Nov '19

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36596 ) Change subject: include: introduce update* for mmio operations ...................................................................... Patch Set 3: (1 comment) https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h File src/include/mmio.h: https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h@19 PS1, Line 19: static __always_inline void update8(const volatile void *addr, uint8_t mask, uint8_t or) > I'm a bit lost now as my parser doesn't seem to work today... […] Yes, that's fine. -- To view, visit https://review.coreboot.org/c/coreboot/+/36596 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9f188d586c09ee9f1e17290563f68970603204fb Gerrit-Change-Number: 36596 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 18:41:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin(a)chromium.org> Comment-In-Reply-To: Michael Niewöhner Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/soc/intel/common/itss: Add support to get IRQ configuration for P...
by Arthur Heymans (Code Review) 07 Nov '19

07 Nov '19

1 0

Change in coreboot[master]: [WIP] arch/x86/smm: Move GNVS prototypes
by Kyösti Mälkki (Code Review) 07 Nov '19

07 Nov '19

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36636 ) Change subject: [WIP] arch/x86/smm: Move GNVS prototypes ...................................................................... [WIP] arch/x86/smm: Move GNVS prototypes Change-Id: Ic8eac39e09d1c36044cb2e2a96edd5c7098b8259 Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/soc/intel/baytrail/include/soc/nvs.h M src/soc/intel/broadwell/include/soc/nvs.h M src/soc/intel/denverton_ns/include/soc/nvs.h M src/soc/intel/fsp_baytrail/include/soc/nvs.h M src/southbridge/intel/bd82x6x/nvs.h M src/southbridge/intel/fsp_rangeley/nvs.h M src/southbridge/intel/ibexpeak/nvs.h M src/southbridge/intel/lynxpoint/nvs.h 8 files changed, 5 insertions(+), 16 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/36636/1 diff --git a/src/soc/intel/baytrail/include/soc/nvs.h b/src/soc/intel/baytrail/include/soc/nvs.h index 08ccbf4..4a89eb9 100644 --- a/src/soc/intel/baytrail/include/soc/nvs.h +++ b/src/soc/intel/baytrail/include/soc/nvs.h @@ -104,9 +104,8 @@ check_member(global_nvs_t, chromeos, GNVS_CHROMEOS_ACPI_OFFSET); void acpi_create_gnvs(global_nvs_t *gnvs); -#ifdef __SMM__ + /* Used in SMM to find the ACPI GNVS address */ global_nvs_t *smm_get_gnvs(void); -#endif #endif /* _BAYTRAIL_NVS_H_ */ diff --git a/src/soc/intel/broadwell/include/soc/nvs.h b/src/soc/intel/broadwell/include/soc/nvs.h index 63c7bde..456fda6 100644 --- a/src/soc/intel/broadwell/include/soc/nvs.h +++ b/src/soc/intel/broadwell/include/soc/nvs.h @@ -96,9 +96,8 @@ check_member(global_nvs_t, chromeos, GNVS_CHROMEOS_ACPI_OFFSET); void acpi_create_gnvs(global_nvs_t *gnvs); -#ifdef __SMM__ + /* Used in SMM to find the ACPI GNVS address */ global_nvs_t *smm_get_gnvs(void); -#endif #endif diff --git a/src/soc/intel/denverton_ns/include/soc/nvs.h b/src/soc/intel/denverton_ns/include/soc/nvs.h index 5a94b24..8d1bc6a 100644 --- a/src/soc/intel/denverton_ns/include/soc/nvs.h +++ b/src/soc/intel/denverton_ns/include/soc/nvs.h @@ -65,9 +65,7 @@ } __packed global_nvs_t; -#ifdef __SMM__ /* Used in SMM to find the ACPI GNVS address */ global_nvs_t *smm_get_gnvs(void); -#endif #endif /* _DENVERTON_NS_NVS_H_ */ diff --git a/src/soc/intel/fsp_baytrail/include/soc/nvs.h b/src/soc/intel/fsp_baytrail/include/soc/nvs.h index d42763a..df2fc60 100644 --- a/src/soc/intel/fsp_baytrail/include/soc/nvs.h +++ b/src/soc/intel/fsp_baytrail/include/soc/nvs.h @@ -65,9 +65,8 @@ } __packed global_nvs_t; void acpi_create_gnvs(global_nvs_t *gnvs); -#ifdef __SMM__ + /* Used in SMM to find the ACPI GNVS address */ global_nvs_t *smm_get_gnvs(void); -#endif #endif /* _BAYTRAIL_NVS_H_ */ diff --git a/src/southbridge/intel/bd82x6x/nvs.h b/src/southbridge/intel/bd82x6x/nvs.h index 6558511..a6b0bdb 100644 --- a/src/southbridge/intel/bd82x6x/nvs.h +++ b/src/southbridge/intel/bd82x6x/nvs.h @@ -155,8 +155,6 @@ } __packed global_nvs_t; check_member(global_nvs_t, chromeos, GNVS_CHROMEOS_ACPI_OFFSET); -#ifdef __SMM__ /* Used in SMM to find the ACPI GNVS address */ global_nvs_t *smm_get_gnvs(void); -#endif void acpi_create_gnvs(global_nvs_t *gnvs); diff --git a/src/southbridge/intel/fsp_rangeley/nvs.h b/src/southbridge/intel/fsp_rangeley/nvs.h index 12de769..47fca68 100644 --- a/src/southbridge/intel/fsp_rangeley/nvs.h +++ b/src/southbridge/intel/fsp_rangeley/nvs.h @@ -146,7 +146,6 @@ } __packed global_nvs_t; void acpi_create_gnvs(global_nvs_t *gnvs); -#ifdef __SMM__ + /* Used in SMM to find the ACPI GNVS address */ global_nvs_t *smm_get_gnvs(void); -#endif diff --git a/src/southbridge/intel/ibexpeak/nvs.h b/src/southbridge/intel/ibexpeak/nvs.h index 36db12c..a956398 100644 --- a/src/southbridge/intel/ibexpeak/nvs.h +++ b/src/southbridge/intel/ibexpeak/nvs.h @@ -153,8 +153,7 @@ } __packed global_nvs_t; check_member(global_nvs_t, chromeos, GNVS_CHROMEOS_ACPI_OFFSET); -#ifdef __SMM__ /* Used in SMM to find the ACPI GNVS address */ global_nvs_t *smm_get_gnvs(void); -#endif + void acpi_create_gnvs(global_nvs_t *gnvs); diff --git a/src/southbridge/intel/lynxpoint/nvs.h b/src/southbridge/intel/lynxpoint/nvs.h index a8f0dea..3aca7bb 100644 --- a/src/southbridge/intel/lynxpoint/nvs.h +++ b/src/southbridge/intel/lynxpoint/nvs.h @@ -130,9 +130,7 @@ } __packed global_nvs_t; check_member(global_nvs_t, chromeos, GNVS_CHROMEOS_ACPI_OFFSET); -#ifdef __SMM__ /* Used in SMM to find the ACPI GNVS address */ global_nvs_t *smm_get_gnvs(void); -#endif void acpi_create_gnvs(global_nvs_t * gnvs); -- To view, visit https://review.coreboot.org/c/coreboot/+/36636 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic8eac39e09d1c36044cb2e2a96edd5c7098b8259 Gerrit-Change-Number: 36636 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: newchange

2 9

Change in coreboot[master]: src/soc/intel/common/itss: Add support to get IRQ configuration for P...
by Arthur Heymans (Code Review) 07 Nov '19

07 Nov '19

1 0

Change in coreboot[master]: src/soc/intel/common/itss: Add support to get IRQ configuration for P...
by Aamir Bohra (Code Review) 07 Nov '19

07 Nov '19

1 0

Change in coreboot[master]: src/soc/intel/common/itss: Add support to get IRQ configuration for P...
by Arthur Heymans (Code Review) 07 Nov '19

07 Nov '19

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 ) Change subject: src/soc/intel/common/itss: Add support to get IRQ configuration for PCI devices ...................................................................... Patch Set 20: (1 comment) > Patch Set 20: > > > Patch Set 20: > > > > > Patch Set 20: > > > > > > > Patch Set 20: > > > > > > > > > Patch Set 20: > > > > > > > > > > > Patch Set 20: > > > > > > > > > > > > > Patch Set 20: > > > > > > > > > > > > > > > Patch Set 20: > > > > > > > > Ok, But this code already creates a irq info data , I am using the same to generate the ACPI code too, I do not need to re-populate the irq matrix. > > > > > > > > > > > > > > This argument sounds to me as if you don't want to use the common facilities we already have because you already wrote your own reimplementation of them. I hope I misunderstand you because that's not very convincing. > > > > > > > > > > > > > > What is missing in acpi_pirq_gen that is present in your code (and can't be added there to avoid having all the common parts twice in our code base)? > > > > > > > > > > > > The intent of this code was to populate irq info based on the device tree selection and pass it to FSP for IRQ configuration based on the irq assignment rules, this implementation is added new(does not exist in acpi_pirq_gen(neither should be scoped in ACPI module)), I think > > > > > > Arthur made a point to re-use APCI generation part from acpi_pirq_gen, to which my response was I did not want to re-populate the irq matrix again, since this code already creates that info. > > > > > > > > > > You don't have to repopulate anything, the matrix is a local structure of that code. You just have to provide a callback function called 'intel_common_map_pirq' which returns a route for a given pin on a given PCI device. Given your current code that should be easy or even trivial to implement. Give the FSP what it needs using this code(and please document it first!) and use the common southbridge code create ACPI _PIR tables. > > > > > > > > > > Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects. > > > > > > > > > > Please consider that reusing common code usually means avoiding pitfalls someone else fell into in the past. > > > > > > > > Hi Arthur, I wanted to understand more on you below comment: > > > > > > > > [Arthur]Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects. > > > > > > > > [Aamir] I do not see multiple entries with same PCI device and int pin mapping , below is the ACPI package that is currently generated with this implementation: > > > > > > > > Name (PIRX, Package (0x11) > > > > { > > > > Package (0x04) > > > > { > > > > 0x0012FFFF, > > > > 0x00, > > > > Zero, > > > > 0x10 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x0014FFFF, > > > > 0x00, > > > > Zero, > > > > 0x10 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x0014FFFF, > > > > 0x01, > > > > Zero, > > > > 0x11 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x0014FFFF, > > > > 0x02, > > > > Zero, > > > > 0x12 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x0015FFFF, > > > > 0x00, > > > > Zero, > > > > 0x10 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x0015FFFF, > > > > 0x01, > > > > Zero, > > > > 0x11 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x0015FFFF, > > > > 0x02, > > > > Zero, > > > > 0x12 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x0016FFFF, > > > > 0x00, > > > > Zero, > > > > 0x10 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x0017FFFF, > > > > 0x00, > > > > Zero, > > > > 0x10 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x0019FFFF, > > > > 0x03, > > > > Zero, > > > > 0x13 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x001DFFFF, > > > > 0x00, > > > > Zero, > > > > 0x10 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x001DFFFF, > > > > 0x01, > > > > Zero, > > > > 0x11 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x001EFFFF, > > > > 0x00, > > > > Zero, > > > > 0x14 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x001EFFFF, > > > > 0x01, > > > > Zero, > > > > 0x15 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x001EFFFF, > > > > 0x02, > > > > Zero, > > > > 0x16 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x001FFFFF, > > > > 0x00, > > > > Zero, > > > > 0x10 > > > > }, > > > > > > > > Package (0x04) > > > > { > > > > 0x001FFFFF, > > > > 0x01, > > > > Zero, > > > > 0x11 > > > > } > > > > }) > > > > > > > > And this is the IRQ table passed to FSP for configuration.(LPSS would not share same IRQs) > > > > https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/bloc… > > > > > > > > Both aligns and do not mismatch, that is the advantage of using the same irq data for creating the APCI package as well. > > > > > > Hi Arthur, did you get a chance to review the acpi package, if no concerns, can you please review your -2 vote? > > > Anyways this CL is needed to populate irq table for FSP to consume. ACPI CL is here: https://review.coreboot.org/c/coreboot/+/34658/6 > > > > No, you are just 'lucky' that there are no double entries. > > > > Please try https://review.coreboot.org/c/coreboot/+/36437 and use that to generate acpi _PRT entries. It should be trivial to extend to use other settings you provide fsp. > > Disagree on that. This implementation runs for all SOC IRQ participating devices just once, for all unique device instances. So there is no scope of getting a double entry for same device and same IRQ pin. You can have multiple functions sharing the same pin, but they *must* share the same route. This code does indeed make sure, like you say all routes are unique, but in a way that is even more wrong than having multiple of the same entries. https://review.coreboot.org/c/coreboot/+/34089/20/src/soc/intel/common/bloc… File src/soc/intel/common/block/itss/irq.c: https://review.coreboot.org/c/coreboot/+/34089/20/src/soc/intel/common/bloc… PS20, Line 50: irq_entry->int_line = int_line++; This seems bad. For a slot you can have up to 7 functions. You can however only have only 4 pins per slot. You end up with multiple lines for for the same pin, which is invalid afaik. -- To view, visit https://review.coreboot.org/c/coreboot/+/34089 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib7066432ff5f0d7017ac5a44922ca69f07da9556 Gerrit-Change-Number: 34089 Gerrit-PatchSet: 20 Gerrit-Owner: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <lean.sheng.tan(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 16:59:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/soc/intel/common/itss: Add support to get IRQ configuration for P...
by Aamir Bohra (Code Review) 07 Nov '19

07 Nov '19

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 ) Change subject: src/soc/intel/common/itss: Add support to get IRQ configuration for PCI devices ...................................................................... Patch Set 20: > Patch Set 20: > > > Patch Set 20: > > > > > Patch Set 20: > > > > > > > Patch Set 20: > > > > > > > > > Patch Set 20: > > > > > > > > > > > Patch Set 20: > > > > > > > > > > > > > Patch Set 20: > > > > > > > Ok, But this code already creates a irq info data , I am using the same to generate the ACPI code too, I do not need to re-populate the irq matrix. > > > > > > > > > > > > This argument sounds to me as if you don't want to use the common facilities we already have because you already wrote your own reimplementation of them. I hope I misunderstand you because that's not very convincing. > > > > > > > > > > > > What is missing in acpi_pirq_gen that is present in your code (and can't be added there to avoid having all the common parts twice in our code base)? > > > > > > > > > > The intent of this code was to populate irq info based on the device tree selection and pass it to FSP for IRQ configuration based on the irq assignment rules, this implementation is added new(does not exist in acpi_pirq_gen(neither should be scoped in ACPI module)), I think > > > > > Arthur made a point to re-use APCI generation part from acpi_pirq_gen, to which my response was I did not want to re-populate the irq matrix again, since this code already creates that info. > > > > > > > > You don't have to repopulate anything, the matrix is a local structure of that code. You just have to provide a callback function called 'intel_common_map_pirq' which returns a route for a given pin on a given PCI device. Given your current code that should be easy or even trivial to implement. Give the FSP what it needs using this code(and please document it first!) and use the common southbridge code create ACPI _PIR tables. > > > > > > > > Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects. > > > > > > > > Please consider that reusing common code usually means avoiding pitfalls someone else fell into in the past. > > > > > > Hi Arthur, I wanted to understand more on you below comment: > > > > > > [Arthur]Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects. > > > > > > [Aamir] I do not see multiple entries with same PCI device and int pin mapping , below is the ACPI package that is currently generated with this implementation: > > > > > > Name (PIRX, Package (0x11) > > > { > > > Package (0x04) > > > { > > > 0x0012FFFF, > > > 0x00, > > > Zero, > > > 0x10 > > > }, > > > > > > Package (0x04) > > > { > > > 0x0014FFFF, > > > 0x00, > > > Zero, > > > 0x10 > > > }, > > > > > > Package (0x04) > > > { > > > 0x0014FFFF, > > > 0x01, > > > Zero, > > > 0x11 > > > }, > > > > > > Package (0x04) > > > { > > > 0x0014FFFF, > > > 0x02, > > > Zero, > > > 0x12 > > > }, > > > > > > Package (0x04) > > > { > > > 0x0015FFFF, > > > 0x00, > > > Zero, > > > 0x10 > > > }, > > > > > > Package (0x04) > > > { > > > 0x0015FFFF, > > > 0x01, > > > Zero, > > > 0x11 > > > }, > > > > > > Package (0x04) > > > { > > > 0x0015FFFF, > > > 0x02, > > > Zero, > > > 0x12 > > > }, > > > > > > Package (0x04) > > > { > > > 0x0016FFFF, > > > 0x00, > > > Zero, > > > 0x10 > > > }, > > > > > > Package (0x04) > > > { > > > 0x0017FFFF, > > > 0x00, > > > Zero, > > > 0x10 > > > }, > > > > > > Package (0x04) > > > { > > > 0x0019FFFF, > > > 0x03, > > > Zero, > > > 0x13 > > > }, > > > > > > Package (0x04) > > > { > > > 0x001DFFFF, > > > 0x00, > > > Zero, > > > 0x10 > > > }, > > > > > > Package (0x04) > > > { > > > 0x001DFFFF, > > > 0x01, > > > Zero, > > > 0x11 > > > }, > > > > > > Package (0x04) > > > { > > > 0x001EFFFF, > > > 0x00, > > > Zero, > > > 0x14 > > > }, > > > > > > Package (0x04) > > > { > > > 0x001EFFFF, > > > 0x01, > > > Zero, > > > 0x15 > > > }, > > > > > > Package (0x04) > > > { > > > 0x001EFFFF, > > > 0x02, > > > Zero, > > > 0x16 > > > }, > > > > > > Package (0x04) > > > { > > > 0x001FFFFF, > > > 0x00, > > > Zero, > > > 0x10 > > > }, > > > > > > Package (0x04) > > > { > > > 0x001FFFFF, > > > 0x01, > > > Zero, > > > 0x11 > > > } > > > }) > > > > > > And this is the IRQ table passed to FSP for configuration.(LPSS would not share same IRQs) > > > https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/bloc… > > > > > > Both aligns and do not mismatch, that is the advantage of using the same irq data for creating the APCI package as well. > > > > Hi Arthur, did you get a chance to review the acpi package, if no concerns, can you please review your -2 vote? > > Anyways this CL is needed to populate irq table for FSP to consume. ACPI CL is here: https://review.coreboot.org/c/coreboot/+/34658/6 > > No, you are just 'lucky' that there are no double entries. > > Please try https://review.coreboot.org/c/coreboot/+/36437 and use that to generate acpi _PRT entries. It should be trivial to extend to use other settings you provide fsp. Disagree on that. This implementation runs for all SOC IRQ participating devices just once, for all unique device instances. So there is no scope of getting a double entry for same device and same IRQ pin. -- To view, visit https://review.coreboot.org/c/coreboot/+/34089 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib7066432ff5f0d7017ac5a44922ca69f07da9556 Gerrit-Change-Number: 34089 Gerrit-PatchSet: 20 Gerrit-Owner: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <lean.sheng.tan(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 16:34:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: security/vboot: Build vboot library with same .a that depthcharge uses
by Tim Wawrzynczak (Code Review) 07 Nov '19

07 Nov '19

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36341 ) Change subject: security/vboot: Build vboot library with same .a that depthcharge uses ...................................................................... Patch Set 18: > Patch Set 18: > > > > This is all good but it still leaves the problem that we need to deal with the TLCL duplicate references (see my comment early on). Are you guys clear on who's going to deal with that (Tim or Joel)? > > Ah, it's only after seeing this CL that I realize the purpose is for handling the duplicate references. Might want to mention that in the vboot_reference CL. Will do -- To view, visit https://review.coreboot.org/c/coreboot/+/36341 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I905b781c3596965ec7ef45a2a7eafe15fdd4d9cc Gerrit-Change-Number: 36341 Gerrit-PatchSet: 18 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Joel Kitching <kitching(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 07 Nov 2019 16:06:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2019