coreboot-gerrit December 2020

coreboot-gerrit@coreboot.org

1 participants
3165 discussions

Change in coreboot[master]: soc/intel: hook up new gpio device in the soc chips
by Michael Niewöhner (Code Review) 14 Dec '20

14 Dec '20

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48583 ) Change subject: soc/intel: hook up new gpio device in the soc chips ...................................................................... Patch Set 7: (1 comment) https://review.coreboot.org/c/coreboot/+/48583/7/src/soc/intel/alderlake/ch… File src/soc/intel/alderlake/chip.c: https://review.coreboot.org/c/coreboot/+/48583/7/src/soc/intel/alderlake/ch… PS7, Line 202: else if (dev->path.type == DEVICE_PATH_GPIO) : dev->ops = &soc_gpio_ops; > > With your code the block gpio driver will get assigned to both gpio devices which is obviously wrong. > > You will have to decorate the gpio device with chip soc/intel/common/block/gpio. Then `intel_gpio_enable()` gets called only for that specific device. Similarly for the other chip_gpio, it's own enable_dev will be called. Thus, there should not be any problem with the above proposal I made. I agree on what Nico says. It's not a chip and we shouldn't fake it. I will try to move the struct. > The `struct gpio_operations` could definitely move to `common/block/`, > that should be easy. Without adding another "chip", you could either make > it `extern` and hook it up here, or call such a intel_gpio_enable() from > here (and each soc). Easy? :'D You have no idea how many different no-working variants I tested to get this moved to block :'D :P ... calling `intel_gpio_enabe` sounds like a good idea, though. -- To view, visit https://review.coreboot.org/c/coreboot/+/48583 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I81dbbf5397b28ffa7537465c53332779245b39f6 Gerrit-Change-Number: 48583 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 14 Dec 2020 23:56:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in em100[master]: trace/console: factor out error handling
by Stefan Reinauer (Code Review) 14 Dec '20

14 Dec '20

Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/48598 ) Change subject: trace/console: factor out error handling ...................................................................... trace/console: factor out error handling Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Change-Id: I313dc6d3433921af38a7deea810dc51a45572ea6 --- M em100.c 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/em100 refs/changes/98/48598/1 diff --git a/em100.c b/em100.c index ca6e683..4b0a121 100644 --- a/em100.c +++ b/em100.c @@ -1233,14 +1233,14 @@ printf(". Press CTL-C to exit.\n\n"); while (!exit_requested && usb_errors < MAX_USB_ERRORS) { + int ret; if (trace) { - if (!read_spi_trace(em100, terminal, - address_offset)) - usb_errors++; + ret = read_spi_trace(em100, terminal, address_offset); } else { - if (!read_spi_terminal(em100, 0)) - usb_errors++; + ret = read_spi_terminal(em100, 0); } + if (ret == 0) + usb_errors++; } if (usb_errors >= MAX_USB_ERRORS) printf("Error: Bailed out with too many USB errors.\n"); -- To view, visit https://review.coreboot.org/c/em100/+/48598 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: em100 Gerrit-Branch: master Gerrit-Change-Id: I313dc6d3433921af38a7deea810dc51a45572ea6 Gerrit-Change-Number: 48598 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newchange

2 2

Change in em100[master]: Fix comment
by Stefan Reinauer (Code Review) 14 Dec '20

14 Dec '20

Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/48588 ) Change subject: Fix comment ...................................................................... Fix comment Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Change-Id: I99d18e12afb0bdd991423305807b3b7a78494605 --- M trace.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/em100 refs/changes/88/48588/1 diff --git a/trace.c b/trace.c index 933575e..22517f9 100644 --- a/trace.c +++ b/trace.c @@ -241,7 +241,7 @@ break; } - /* set up address if used by this command*/ + /* set up address if used by this command */ if (!spi_cmd_vals->uses_address) { j = 1; /* skip command byte */ } else { -- To view, visit https://review.coreboot.org/c/em100/+/48588 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: em100 Gerrit-Branch: master Gerrit-Change-Id: I99d18e12afb0bdd991423305807b3b7a78494605 Gerrit-Change-Number: 48588 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newchange

2 3

Change in coreboot[master]: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
by Shreesh Chhabbi (Code Review) 14 Dec '20

14 Dec '20

Hello Shreesh Chhabbi, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/48286 to review the following change. Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ...................................................................... src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake. Bug=b:171601324 BRANCH=volteer Test=[to be updated] Signed-off-by: Shreesh Chhabbi <shreesh.chhabbi(a)intel.corp-partner.google.com> Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi <shreesh.chhabbi(a)intel.corp-partner.google.com> --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 38 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/1 diff --git a/src/soc/intel/common/block/cpu/Kconfig b/src/soc/intel/common/block/cpu/Kconfig index 912760e..2b630d0 100644 --- a/src/soc/intel/common/block/cpu/Kconfig +++ b/src/soc/intel/common/block/cpu/Kconfig @@ -51,6 +51,14 @@ ENHANCED NEM guarantees that modified data is always kept in cache while clean data is replaced. +config CAR_HAS_SF_MASKS + bool + depends on INTEL_CAR_NEM_ENHANCED + help + In the case of non-inclusive cache architecture Snoop Filter MSR + IA32_L3_SF_MASK_x programming is required along with the data ways. + This is applicable for TGL and beyond. + config COS_MAPPED_TO_MSB bool depends on INTEL_CAR_NEM_ENHANCED diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S index 7408822..93f5f95 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S @@ -414,6 +414,35 @@ set_eviction_mask: mov %ebx, %ecx /* back up the number of ways */ mov %eax, %ebx /* back up the non-eviction mask*/ + mov %ecx, %edi /* back up the number of ways */ + +#if CONFIG(CAR_HAS_SF_MASKS) + /* + * If total number of available ways is X, SF mask is ((0x1 << X*2) - 1) + */ + mov $0x02, %eax + mul %ecx + mov %eax, %ecx + mov $0x01, %eax + shl %cl, %eax + subl $0x01, %eax /* Holds the SF mask */ + /* + * Program MSR 0x1891 IA32_CR_SF_QOS_MASK_1 with + * total number of LLC ways + */ + movl $IA32_CR_SF_QOS_MASK_1, %ecx + xorl %edx, %edx + wrmsr + /* + * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with + * total number of LLC ways + */ + movl $IA32_CR_SF_QOS_MASK_2, %ecx + xorl %edx, %edx + wrmsr + mov %edi, %ecx /* Restore number of ways */ +#endif + /* * Program MSR 0xC91 IA32_L3_MASK_1 * This MSR contain one bit per each way of LLC diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index f53c18a..dd14ad2 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -24,6 +24,7 @@ select HAVE_SMI_HANDLER select IDT_IN_EVERY_STAGE select INTEL_CAR_NEM_ENHANCED if !INTEL_CAR_NEM + select CAR_HAS_SF_MASKS if INTEL_CAR_NEM_ENHANCED select COS_MAPPED_TO_MSB if INTEL_CAR_NEM_ENHANCED select INTEL_GMA_ACPI select INTEL_GMA_ADD_VBT if RUN_FSP_GOP -- To view, visit https://review.coreboot.org/c/coreboot/+/48286 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Gerrit-Change-Number: 48286 Gerrit-PatchSet: 1 Gerrit-Owner: Shreesh Chhabbi <shreesh.chhabbi(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Shreesh Chhabbi <shreesh.chhabbi(a)intel.corp-partner.google.com> Gerrit-MessageType: newchange

5 80

Change in coreboot[master]: soc/intel: Remove INTEL_CAR_NEM_ENHANCED_V2 config option
by Shreesh Chhabbi (Code Review) 14 Dec '20

14 Dec '20

Hello Shreesh Chhabbi, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/48284 to review the following change. Change subject: soc/intel: Remove INTEL_CAR_NEM_ENHANCED_V2 config option ...................................................................... soc/intel: Remove INTEL_CAR_NEM_ENHANCED_V2 config option SF Mask MSRs' Programming which was done under this config selection will be moved under a new config option called CAR_HAS_SF_MASKS. This segregates the eNEM programming sequence based on sub features supported in each processor. Change-Id: If4d8d1ec52b7b79965fe1a957c48f571ec56dc63 Signed-off-by: Shreesh Chhabbi <shreesh.chhabbi(a)intel.corp-partner.google.com> --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/jasperlake/Kconfig M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig 8 files changed, 8 insertions(+), 31 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/48284/1 diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index f427340..06f8c2e 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -315,7 +315,7 @@ config USE_CANNONLAKE_CAR_NEM_ENHANCED bool "Enhanced Non-evict mode" select SOC_INTEL_COMMON_BLOCK_CAR - select USE_CAR_NEM_ENHANCED_V1 + select INTEL_CAR_NEM_ENHANCED help A current limitation of NEM (Non-Evict mode) is that code and data sizes are derived from the requirement to not write out any modified diff --git a/src/soc/intel/common/block/cpu/Kconfig b/src/soc/intel/common/block/cpu/Kconfig index 9023b58..912760e 100644 --- a/src/soc/intel/common/block/cpu/Kconfig +++ b/src/soc/intel/common/block/cpu/Kconfig @@ -51,21 +51,6 @@ ENHANCED NEM guarantees that modified data is always kept in cache while clean data is replaced. -config USE_CAR_NEM_ENHANCED_V1 - bool - select INTEL_CAR_NEM_ENHANCED - help - This config supports INTEL_CAR_NEM_ENHANCED mode on - SKL, KBL, CNL, WHL, CML and ICL and JSL platforms. - -config USE_CAR_NEM_ENHANCED_V2 - bool - select INTEL_CAR_NEM_ENHANCED - select COS_MAPPED_TO_MSB - help - This config supports INTEL_CAR_NEM_ENHANCED mode on - TGL platform. - config COS_MAPPED_TO_MSB bool depends on INTEL_CAR_NEM_ENHANCED diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S index 167342f..7408822 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S @@ -415,7 +415,7 @@ mov %ebx, %ecx /* back up the number of ways */ mov %eax, %ebx /* back up the non-eviction mask*/ /* - * Set MSR 0xC91 IA32_L3_MASK_1 or MSR 0x1891 IA32_CR_SF_QOS_MASK_1 + * Program MSR 0xC91 IA32_L3_MASK_1 * This MSR contain one bit per each way of LLC * - If this bit is '0' - the way is protected from eviction * - If this bit is '1' - the way is not protected from eviction @@ -428,26 +428,18 @@ xor $~0, %eax /* invert 32 bits */ and %ecx, %eax -#if CONFIG(USE_CAR_NEM_ENHANCED_V1) movl $IA32_L3_MASK_1, %ecx -#elif CONFIG(USE_CAR_NEM_ENHANCED_V2) - movl $IA32_CR_SF_QOS_MASK_1, %ecx -#endif xorl %edx, %edx wrmsr /* - * Set MSR 0xC92 IA32_L3_MASK_1 or MSR 0x1892 IA32_CR_SF_QOS_MASK_2 + * Program MSR 0xC92 IA32_L3_MASK_2 * This MSR contain one bit per each way of LLC * - If this bit is '0' - the way is protected from eviction * - If this bit is '1' - the way is not protected from eviction */ mov %ebx, %eax -#if CONFIG(USE_CAR_NEM_ENHANCED_V1) movl $IA32_L3_MASK_2, %ecx -#elif CONFIG(USE_CAR_NEM_ENHANCED_V2) - movl $IA32_CR_SF_QOS_MASK_2, %ecx -#endif xorl %edx, %edx wrmsr /* diff --git a/src/soc/intel/denverton_ns/Kconfig b/src/soc/intel/denverton_ns/Kconfig index 4981205..4fe491f 100644 --- a/src/soc/intel/denverton_ns/Kconfig +++ b/src/soc/intel/denverton_ns/Kconfig @@ -170,7 +170,7 @@ config USE_DENVERTON_NS_CAR_NEM_ENHANCED bool "Enhanced Non-evict mode" select SOC_INTEL_COMMON_BLOCK_CAR - select USE_CAR_NEM_ENHANCED_V1 + select INTEL_CAR_NEM_ENHANCED help A current limitation of NEM (Non-Evict mode) is that code and data sizes are derived from the requirement to not write out any modified cache line. diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 52e9a74..fee52e6 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -23,6 +23,7 @@ select INTEL_DESCRIPTOR_MODE_CAPABLE select HAVE_SMI_HANDLER select IDT_IN_EVERY_STAGE + select INTEL_CAR_NEM_ENHANCED select INTEL_GMA_ACPI select INTEL_GMA_ADD_VBT if RUN_FSP_GOP select IOAPIC @@ -64,7 +65,6 @@ select DISPLAY_FSP_VERSION_INFO select HECI_DISABLE_USING_SMM select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI - select USE_CAR_NEM_ENHANCED_V1 config DCACHE_RAM_BASE default 0xfef00000 diff --git a/src/soc/intel/jasperlake/Kconfig b/src/soc/intel/jasperlake/Kconfig index 294f19f8..0a15f05 100644 --- a/src/soc/intel/jasperlake/Kconfig +++ b/src/soc/intel/jasperlake/Kconfig @@ -23,6 +23,7 @@ select INTEL_DESCRIPTOR_MODE_CAPABLE select HAVE_SMI_HANDLER select IDT_IN_EVERY_STAGE + select INTEL_CAR_NEM_ENHANCED select INTEL_GMA_ACPI select INTEL_GMA_ADD_VBT if RUN_FSP_GOP select IOAPIC @@ -61,7 +62,6 @@ select TSC_MONOTONIC_TIMER select UDELAY_TSC select UDK_202005_BINDING - select USE_CAR_NEM_ENHANCED_V1 select DISPLAY_FSP_VERSION_INFO select HECI_DISABLE_USING_SMM diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index ba3af84..26ada09 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -30,6 +30,7 @@ select GENERIC_GPIO_LIB select HAVE_FSP_GOP select HAVE_FSP_LOGO_SUPPORT + select INTEL_CAR_NEM_ENHANCED select INTEL_DESCRIPTOR_MODE_CAPABLE select HAVE_SMI_HANDLER select INTEL_GMA_ACPI @@ -77,7 +78,6 @@ select TSC_SYNC_MFENCE select UDELAY_TSC select UDK_2015_BINDING - select USE_CAR_NEM_ENHANCED_V1 config FSP_HYPERTHREADING bool "Enable Hyper-Threading" diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index b97b92e..f53c18a 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -23,7 +23,7 @@ select INTEL_DESCRIPTOR_MODE_CAPABLE select HAVE_SMI_HANDLER select IDT_IN_EVERY_STAGE - select USE_CAR_NEM_ENHANCED_V1 if !INTEL_CAR_NEM + select INTEL_CAR_NEM_ENHANCED if !INTEL_CAR_NEM select COS_MAPPED_TO_MSB if INTEL_CAR_NEM_ENHANCED select INTEL_GMA_ACPI select INTEL_GMA_ADD_VBT if RUN_FSP_GOP -- To view, visit https://review.coreboot.org/c/coreboot/+/48284 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If4d8d1ec52b7b79965fe1a957c48f571ec56dc63 Gerrit-Change-Number: 48284 Gerrit-PatchSet: 1 Gerrit-Owner: Shreesh Chhabbi <shreesh.chhabbi(a)intel.com> Gerrit-Reviewer: Shreesh Chhabbi <shreesh.chhabbi(a)intel.corp-partner.google.com> Gerrit-MessageType: newchange

4 11

Change in coreboot[master]: soc/intel: hook up new gpio device in the soc chips
by Furquan Shaikh (Code Review) 14 Dec '20

14 Dec '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48583 ) Change subject: soc/intel: hook up new gpio device in the soc chips ...................................................................... Patch Set 7: (1 comment) https://review.coreboot.org/c/coreboot/+/48583/7/src/soc/intel/alderlake/ch… File src/soc/intel/alderlake/chip.c: https://review.coreboot.org/c/coreboot/+/48583/7/src/soc/intel/alderlake/ch… PS7, Line 202: else if (dev->path.type == DEVICE_PATH_GPIO) : dev->ops = &soc_gpio_ops; > With your code the block gpio driver will get assigned to both gpio devices which is obviously wrong. You will have to decorate the gpio device with chip soc/intel/common/block/gpio. Then `intel_gpio_enable()` gets called only for that specific device. Similarly for the other chip_gpio, it's own enable_dev will be called. Thus, there should not be any problem with the above proposal I made. -- To view, visit https://review.coreboot.org/c/coreboot/+/48583 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I81dbbf5397b28ffa7537465c53332779245b39f6 Gerrit-Change-Number: 48583 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 14 Dec 2020 22:05:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel: hook up new gpio device in the soc chips
by Nico Huber (Code Review) 14 Dec '20

14 Dec '20

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48583 ) Change subject: soc/intel: hook up new gpio device in the soc chips ...................................................................... Patch Set 7: (1 comment) https://review.coreboot.org/c/coreboot/+/48583/7/src/soc/intel/alderlake/ch… File src/soc/intel/alderlake/chip.c: https://review.coreboot.org/c/coreboot/+/48583/7/src/soc/intel/alderlake/ch… PS7, Line 202: else if (dev->path.type == DEVICE_PATH_GPIO) : dev->ops = &soc_gpio_ops; > With your code the block gpio driver will get assigned to both gpio devices which is obviously wrong. Only if intel_gpio_enable() gets called (it shouldn't be). But as I mentioned elsewhere, I'm not a fan of fake chips. The `struct gpio_operations` could definitely move to `common/block/`, that should be easy. Without adding another "chip", you could either make it `extern` and hook it up here, or call such a intel_gpio_enable() from here (and each soc). -- To view, visit https://review.coreboot.org/c/coreboot/+/48583 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I81dbbf5397b28ffa7537465c53332779245b39f6 Gerrit-Change-Number: 48583 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 14 Dec 2020 22:02:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/intel/usb4: Add Mux Ack driver
by Brandon Breitenstein (Code Review) 14 Dec '20

14 Dec '20

Brandon Breitenstein has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48253 ) Change subject: drivers/intel/usb4: Add Mux Ack driver ...................................................................... drivers/intel/usb4: Add Mux Ack driver There are some syncronization issues between EC and Kernel that can cause unwanted side effects in the USB4/TCSS flows if left alone. This driver adds ACPI methods to set a gpio so that EC and Kernel can remain in sync on PMC requests. Change-Id: Ib584150a0c12bae34ebe8d297288425635873919 Signed-off-by: Brandon Breitenstein <brandon.breitenstein(a)intel.com> --- A src/drivers/intel/usb4/mux_ack/Kconfig A src/drivers/intel/usb4/mux_ack/Makefile.inc A src/drivers/intel/usb4/mux_ack/ack.c A src/drivers/intel/usb4/mux_ack/chip.h 4 files changed, 160 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/48253/1 diff --git a/src/drivers/intel/usb4/mux_ack/Kconfig b/src/drivers/intel/usb4/mux_ack/Kconfig new file mode 100644 index 0000000..3528a9f --- /dev/null +++ b/src/drivers/intel/usb4/mux_ack/Kconfig @@ -0,0 +1,8 @@ +config DRIVERS_INTEL_USB4_MUX_ACK + bool + depends on HAVE_ACPI_TABLES + help + The mux ack driver is required when syncronization is needed + between the EC and the Kernel on the state transitions of the + Type-C muxes. If your mainboard is utilizing TCSS controlled + muxes select this driver. diff --git a/src/drivers/intel/usb4/mux_ack/Makefile.inc b/src/drivers/intel/usb4/mux_ack/Makefile.inc new file mode 100644 index 0000000..86c43ee --- /dev/null +++ b/src/drivers/intel/usb4/mux_ack/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_INTEL_USB4_MUX_ACK) += ack.c diff --git a/src/drivers/intel/usb4/mux_ack/ack.c b/src/drivers/intel/usb4/mux_ack/ack.c new file mode 100644 index 0000000..0dc186a --- /dev/null +++ b/src/drivers/intel/usb4/mux_ack/ack.c @@ -0,0 +1,137 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <acpi/acpigen.h> +#include <acpi/acpi_device.h> +#include <console/console.h> +#include <device/device.h> +#include <device/path.h> +#include <gpio.h> +#include <string.h> +#include "chip.h" + +/* Unique ID for the ACK _DSM. */ +#define INTEL_USB4_MUX_ACK_DSM_UUID "5e7fb958-b2c5-49dc-90b9-f95b1275e824" + +/* + * Arg0: UUID + * Arg1: Revision ID (set to 1) + * Arg2: Function Index + * 0: Query command implemented + * 1: Query current ACK state + * 2: Set ACK state + * Arg3: A package containing parameters for the function specified by the UUID + * revision ID and function index. + * Function 2: Value to set ACK GPIO + */ + +static void usb4_mux_ack_cb_standard_query(void *arg) +{ + /* + * ToInteger (Arg1, Local2) + * If (Local2 == 1) { + * Return(Buffer() {0x07}) + * } + * Return (Buffer() {0x01}) + */ +acpigen_write_to_integer(ARG1_OP, LOCAL2_OP); + + /* Revision 1 supports 2 Functions beyond the standard query */ + acpigen_write_if_lequal_op_int(LOCAL2_OP, 1); + acpigen_write_return_singleton_buffer(0x07); + acpigen_pop_len(); /* If */ + + /* Other revisions support no additional functions */ + acpigen_write_return_singleton_buffer(0); +} + +static void usb4_mux_ack_cb_get_ack_state(void *arg) +{ + struct acpi_gpio *ack_gpio = arg; + + /* + * // Read ack gpio into Local0 + * Store (\_SB.PCI0.GTXS (ack_gpio), Local0) + * Return (Local0) + */ + acpigen_get_tx_gpio(ack_gpio); + acpigen_write_return_op(LOCAL0_OP); +} + +static void usb4_mux_ack_cb_set_ack_state(void *arg) +{ + struct acpi_gpio *ack_gpio = arg; + + /* + * // Get argument for on/off from Arg3[0] + * Local0 = DeRefOf (Arg3[0]) + */ + acpigen_get_package_op_element(ARG3_OP, 0, LOCAL0_OP); + + /* + * If (Local0 == 0) { + * // Turn ACK Off + * \_SB.PCI0.CTXS (ack_gpio) + * } + */ + acpigen_write_if_lequal_op_int(LOCAL0_OP, 0); + acpigen_disable_tx_gpio(ack_gpio); + acpigen_pop_len(); /* If */ + + /* + * Else { + * // Turn ACK on + * \_SB.PCI0.STXS (ack_gpio) + * } + */ + acpigen_write_else(); + acpigen_enable_tx_gpio(ack_gpio); + acpigen_pop_len(); + + /* Return (Zero) */ + acpigen_write_return_integer(0); +} + +static void (*usb4_mux_ack_callbacks[3])(void *) = { + usb4_mux_ack_cb_standard_query, /* Function 0 */ + usb4_mux_ack_cb_get_ack_state, /* Function 1 */ + usb4_mux_ack_cb_set_ack_state, /* Function 2 */ +}; + +static void usb4_mux_ack_fill_ssdt(const struct device *dev) +{ + const struct drivers_intel_usb4_mux_ack_config *config = dev->chip_info; + const char *scope = acpi_device_scope(dev); + + if (!scope || !config) + return; + + if (!config->ack_gpio.pin_count) { + printk(BIOS_ERR, "%s: Power GPIO required for %s\n", __func__, dev_path(dev)); + return; + } + + /* Write the _DSM that toggles ack with provided GPIO. */ + acpigen_write_scope(scope); + acpigen_write_dsm(INTEL_USB4_MUX_ACK_DSM_UUID, usb4_mux_ack_callbacks, + ARRAY_SIZE(usb4_mux_ack_callbacks), (void *)&config->ack_gpio); + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s at %s\n", acpi_device_path(dev), dev->chip_ops->name, + dev_path(dev)); +} + +static struct device_operations usb4_mux_ack_dev_ops = { + .read_resources = noop_read_resources, + .set_resources = noop_set_resources, + .acpi_fill_ssdt = usb4_mux_ack_fill_ssdt, +}; + +static void usb4_mux_ack_enable(struct device *dev) +{ + dev->ops = &usb4_mux_ack_dev_ops; +} + +struct chip_operations drivers_intel_usb4_mux_ack_ops = { + CHIP_NAME("Intel USB4 Mux Ack") + .enable_dev = usb4_mux_ack_enable +}; diff --git a/src/drivers/intel/usb4/mux_ack/chip.h b/src/drivers/intel/usb4/mux_ack/chip.h new file mode 100644 index 0000000..c74df6a --- /dev/null +++ b/src/drivers/intel/usb4/mux_ack/chip.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef __DRIVERS_INTEL_USB4_ACK_H__ +#define __DRIVERS_INTEL_USB4_ACK_H__ + +#include <acpi/acpi_device.h> + +struct drivers_intel_usb4_mux_ack_config { + /* GPIO used to tell EC that a mux mode request has been completed. */ + struct acpi_gpio ack_gpio; +}; + +#endif /* __DRIVERS_INTEL_USB4_ACK_H__ */ + -- To view, visit https://review.coreboot.org/c/coreboot/+/48253 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib584150a0c12bae34ebe8d297288425635873919 Gerrit-Change-Number: 48253 Gerrit-PatchSet: 1 Gerrit-Owner: Brandon Breitenstein <brandon.breitenstein(a)intel.com> Gerrit-MessageType: newchange

2 3

Change in coreboot[master]: device + util/sconfig: introduce new device `gpio`
by Nico Huber (Code Review) 14 Dec '20

14 Dec '20

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48582 ) Change subject: device + util/sconfig: introduce new device `gpio` ...................................................................... Patch Set 4: (3 comments) https://review.coreboot.org/c/coreboot/+/48582/4/src/device/device_const.c File src/device/device_const.c: https://review.coreboot.org/c/coreboot/+/48582/4/src/device/device_const.c@… PS4, Line 166: generic.id `gpio.id`? https://review.coreboot.org/c/coreboot/+/48582/4/src/device/device_util.c File src/device/device_util.c: https://review.coreboot.org/c/coreboot/+/48582/4/src/device/device_util.c@1… PS4, Line 134: generic.id `gpio.id`? https://review.coreboot.org/c/coreboot/+/48582/4/src/device/device_util.c@2… PS4, Line 230: %d.%d", dev->path.generic.id I guess this should be just `%d", dev->path.gpio.id);`? Also, do I miss something? is `-Wformat` not enabled? -- To view, visit https://review.coreboot.org/c/coreboot/+/48582 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic4572ad8b37bd1afd2fb213b2c67fb8aec536786 Gerrit-Change-Number: 48582 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 14 Dec 2020 21:44:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: sb/intel/common/smbus_ops.c: Clean up read resources
by Angel Pons (Code Review) 14 Dec '20

14 Dec '20

Hello Nico Huber, Arthur Heymans, Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/48538 to review the following change. Change subject: sb/intel/common/smbus_ops.c: Clean up read resources ...................................................................... sb/intel/common/smbus_ops.c: Clean up read resources UNTESTED. Change-Id: I1cb05131a82ebb7c45827eff8e09e445d9c695b3 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/southbridge/intel/common/smbus_ops.c 1 file changed, 2 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/48538/1 diff --git a/src/southbridge/intel/common/smbus_ops.c b/src/southbridge/intel/common/smbus_ops.c index b0ecc1a..46fc58a 100644 --- a/src/southbridge/intel/common/smbus_ops.c +++ b/src/southbridge/intel/common/smbus_ops.c @@ -67,17 +67,12 @@ void smbus_read_resources(struct device *dev) { + pci_dev_read_resources(dev); + struct resource *res = new_resource(dev, PCI_BASE_ADDRESS_4); res->base = CONFIG_FIXED_SMBUS_IO_BASE; res->size = 32; res->limit = res->base + res->size - 1; res->flags = IORESOURCE_IO | IORESOURCE_FIXED | IORESOURCE_RESERVE | IORESOURCE_STORED | IORESOURCE_ASSIGNED; - - /* The memory BAR does not exist for ICH7 and earlier */ - if (CONFIG(SOUTHBRIDGE_INTEL_I82801GX)) - return; - - /* Also add MMIO resource */ - res = pci_get_resource(dev, PCI_BASE_ADDRESS_0); } -- To view, visit https://review.coreboot.org/c/coreboot/+/48538 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1cb05131a82ebb7c45827eff8e09e445d9c695b3 Gerrit-Change-Number: 48538 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

2 5

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2020