coreboot-gerrit January 2025

coreboot-gerrit@coreboot.org

1 participants
3269 discussions

[XS] Change in coreboot[main]: device/pci_ids: Add Intel Panther Lake device IDs for Bluetooth CNVi
by Cliff Huang (Code Review) 13 Jan '25

13 Jan '25

Attention is currently required from: Bora Guvendik, Jérémy Compostella, Zhixing Ma. Cliff Huang has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/85959?usp=email ) Change subject: device/pci_ids: Add Intel Panther Lake device IDs for Bluetooth CNVi ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/85959?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I45b42b0694d530763d4cd321aefc64141d088e2b Gerrit-Change-Number: 85959 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Comment-Date: Mon, 13 Jan 2025 18:41:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[XS] Change in coreboot[main]: device/pci_ids: Add Intel Panther Lake device IDs for Bluetooth CNVi
by Jamie Ryu (Code Review) 13 Jan '25

13 Jan '25

Attention is currently required from: Bora Guvendik, Cliff Huang, Jérémy Compostella, Zhixing Ma. Jamie Ryu has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/85959?usp=email ) Change subject: device/pci_ids: Add Intel Panther Lake device IDs for Bluetooth CNVi ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/85959?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I45b42b0694d530763d4cd321aefc64141d088e2b Gerrit-Change-Number: 85959 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Comment-Date: Mon, 13 Jan 2025 18:40:38 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[XS] Change in coreboot[main]: device/pci_ids: Add Intel Panther Lake device IDs for Bluetooth CNVi
by Jérémy Compostella (Code Review) 13 Jan '25

13 Jan '25

Jérémy Compostella has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/85959?usp=email ) Change subject: device/pci_ids: Add Intel Panther Lake device IDs for Bluetooth CNVi ...................................................................... device/pci_ids: Add Intel Panther Lake device IDs for Bluetooth CNVi This commit introduces the missing PCI device IDs for Panther Lake CNVi Bluetooth devices. These IDs are listed in document #815002 - Panther Lake U/H Processor - External Design Specification Volume 1. TEST=The CNVB device is now present in the ACPI SSDT table when the cnvi_bluetooth option is enabled. Change-Id: I45b42b0694d530763d4cd321aefc64141d088e2b Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> --- M src/include/device/pci_ids.h M src/soc/intel/common/block/cnvi/cnvi.c 2 files changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/85959/1 diff --git a/src/include/device/pci_ids.h b/src/include/device/pci_ids.h index 4279588..60a7d17 100644 --- a/src/include/device/pci_ids.h +++ b/src/include/device/pci_ids.h @@ -4909,10 +4909,12 @@ #define PCI_DID_INTEL_PTL_H_CNVI_WIFI_1 0xe441 #define PCI_DID_INTEL_PTL_H_CNVI_WIFI_2 0xe442 #define PCI_DID_INTEL_PTL_H_CNVI_WIFI_3 0xe443 +#define PCI_DID_INTEL_PTL_H_CNVI_BT 0xe476 #define PCI_DID_INTEL_PTL_U_H_CNVI_WIFI_0 0xe340 #define PCI_DID_INTEL_PTL_U_H_CNVI_WIFI_1 0xe341 #define PCI_DID_INTEL_PTL_U_H_CNVI_WIFI_2 0xe342 #define PCI_DID_INTEL_PTL_U_H_CNVI_WIFI_3 0xe343 +#define PCI_DID_INTEL_PTL_U_H_CNVI_BT 0xe376 /* Platform Security Engine */ #define PCI_DID_INTEL_LNL_PSE0 0xa862 diff --git a/src/soc/intel/common/block/cnvi/cnvi.c b/src/soc/intel/common/block/cnvi/cnvi.c index 93f0cae..94fe21f 100644 --- a/src/soc/intel/common/block/cnvi/cnvi.c +++ b/src/soc/intel/common/block/cnvi/cnvi.c @@ -470,6 +470,8 @@ }; static const unsigned short bt_pci_device_ids[] = { + PCI_DID_INTEL_PTL_H_CNVI_BT, + PCI_DID_INTEL_PTL_U_H_CNVI_BT, PCI_DID_INTEL_TGL_CNVI_BT_0, PCI_DID_INTEL_TGL_CNVI_BT_1, PCI_DID_INTEL_TGL_CNVI_BT_2, -- To view, visit https://review.coreboot.org/c/coreboot/+/85959?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I45b42b0694d530763d4cd321aefc64141d088e2b Gerrit-Change-Number: 85959 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>

1 0

[S] Change in coreboot[main]: mb/google/fatcat/var/fatcat: Refactor GPIO programming for UFS support
by Subrata Banik (Code Review) 13 Jan '25

13 Jan '25

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/85958?usp=email ) Change subject: mb/google/fatcat/var/fatcat: Refactor GPIO programming for UFS support ...................................................................... mb/google/fatcat/var/fatcat: Refactor GPIO programming for UFS support Refactor GPIO programming to support UFS storage on the fatcat platform. - Add pad configurations for UFS in `fw_config.c`. - Update `fw_config_configure_pre_mem_gpio()` and `fw_config_gpio_padbased_override()` to include UFS support. - Remove redundant UFS pad configuration from `gpio.c`. TEST=Able to build and boot from UFS device on google/fatcat. Change-Id: I09331d75501977d89592d1a70d5b0dca271f8747 Signed-off-by: Subrata Banik <subratabanik(a)google.com> --- M src/mainboard/google/fatcat/variants/fatcat/fw_config.c M src/mainboard/google/fatcat/variants/fatcat/gpio.c 2 files changed, 10 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/85958/1 diff --git a/src/mainboard/google/fatcat/variants/fatcat/fw_config.c b/src/mainboard/google/fatcat/variants/fatcat/fw_config.c index ac2155f..0ba6048 100644 --- a/src/mainboard/google/fatcat/variants/fatcat/fw_config.c +++ b/src/mainboard/google/fatcat/variants/fatcat/fw_config.c @@ -210,6 +210,11 @@ PAD_CFG_GPO(GPP_B09, 1, PLTRST), }; +static const struct pad_config ufs_enable_pads[] = { + /* GPP_D21: GPP_D21_UFS_REFCLK */ + PAD_CFG_NF(GPP_D21, NONE, DEEP, NF1), +}; + /* Gen5 NVME: at the bottom M.2 slot */ static const struct pad_config pre_mem_gen5_ssd_pwr_pads[] = { /* GPP_B16: GEN5_SSD_PWREN */ @@ -484,12 +489,9 @@ GPIO_CONFIGURE_PADS(pre_mem_gen4_ssd_pwr_pads); } else if (fw_config_probe(FW_CONFIG(STORAGE, STORAGE_NVME_GEN5))) { GPIO_CONFIGURE_PADS(pre_mem_gen5_ssd_pwr_pads); - /* TODO: else if (fw_config_probe(FW_CONFIG(STORAGE, STORAGE_UFS))) - */ } else if (fw_config_probe(FW_CONFIG(STORAGE, STORAGE_UNKNOWN))) { GPIO_CONFIGURE_PADS(pre_mem_gen4_ssd_pwr_pads); GPIO_CONFIGURE_PADS(pre_mem_gen5_ssd_pwr_pads); - /* TODO for UFS */ } if (!fw_config_probe(FW_CONFIG(SD, SD_NONE))) @@ -524,12 +526,12 @@ GPIO_PADBASED_OVERRIDE(padbased_table, gen4_ssd_pads); } else if (fw_config_probe(FW_CONFIG(STORAGE, STORAGE_NVME_GEN5))) { GPIO_PADBASED_OVERRIDE(padbased_table, gen5_ssd_pads); - /* TODO: else if (fw_config_probe(FW_CONFIG(STORAGE, STORAGE_UFS))) - */ + } else if (fw_config_probe(FW_CONFIG(STORAGE, STORAGE_UFS))) { + GPIO_PADBASED_OVERRIDE(padbased_table, ufs_enable_pads); } else if (fw_config_probe(FW_CONFIG(STORAGE, STORAGE_UNKNOWN))) { GPIO_PADBASED_OVERRIDE(padbased_table, gen4_ssd_pads); GPIO_PADBASED_OVERRIDE(padbased_table, gen5_ssd_pads); - /* TODO for UFS */ + GPIO_PADBASED_OVERRIDE(padbased_table, ufs_enable_pads); } if (fw_config_probe(FW_CONFIG(AUDIO, AUDIO_NONE))) diff --git a/src/mainboard/google/fatcat/variants/fatcat/gpio.c b/src/mainboard/google/fatcat/variants/fatcat/gpio.c index d6fea09..cf1e040 100644 --- a/src/mainboard/google/fatcat/variants/fatcat/gpio.c +++ b/src/mainboard/google/fatcat/variants/fatcat/gpio.c @@ -175,8 +175,8 @@ PAD_CFG_NF(GPP_D18, NONE, DEEP, NF1), /* GPP_D20: CSE_EARLY_SW */ PAD_CFG_GPI_SCI_HIGH(GPP_D20, NONE, DEEP, LEVEL), - /* GPP_D21: GPP_D21_UFS_REFCLK */ - PAD_CFG_NF(GPP_D21, NONE, DEEP, NF1), + /* GPP_D21: NC */ + PAD_NC(GPP_D21, NONE), /* GPP_D22: BPKI3C_SDA */ PAD_CFG_NF(GPP_D22, NONE, DEEP, NF1), /* GPP_D23: BPKI3C_SCL */ -- To view, visit https://review.coreboot.org/c/coreboot/+/85958?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I09331d75501977d89592d1a70d5b0dca271f8747 Gerrit-Change-Number: 85958 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>

1 0

[S] Change in coreboot[main]: soc/intel/common: Simply code accessing scaling factors
by Jérémy Compostella (Code Review) 13 Jan '25

13 Jan '25

Jérémy Compostella has submitted this change. ( https://review.coreboot.org/c/coreboot/+/85897?usp=email ) Change subject: soc/intel/common: Simply code accessing scaling factors ...................................................................... soc/intel/common: Simply code accessing scaling factors This commit streamlines the call to the soc_read_core_scaling_factors() function. When runtime access to the core scaling factors is not available, a static fallback is used based on the CONFIG_SOC_INTEL_PERFORMANCE_CORE_SCALE_FACTOR and CONFIG_SOC_INTEL_EFFICIENT_CORE_SCALE_FACTOR options. TEST=Successfully read performance and efficient scaling factors on a fatcat board. Change-Id: I62e903bea07f2981dfcbaf61d3b918e7c332afc5 Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> Suggested-by: Subrata Banik <subratabanik(a)google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/85897 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Subrata Banik <subratabanik(a)google.com> --- M src/soc/intel/common/block/acpi/cpu_hybrid.c M src/soc/intel/common/block/include/intelblocks/acpi.h 2 files changed, 20 insertions(+), 16 deletions(-) Approvals: Subrata Banik: Looks good to me, approved build bot (Jenkins): Verified diff --git a/src/soc/intel/common/block/acpi/cpu_hybrid.c b/src/soc/intel/common/block/acpi/cpu_hybrid.c index 2c3b84c..ee4777a 100644 --- a/src/soc/intel/common/block/acpi/cpu_hybrid.c +++ b/src/soc/intel/common/block/acpi/cpu_hybrid.c @@ -110,22 +110,8 @@ u8 max_non_turbo_ratio = cpu_get_max_non_turbo_ratio(); static u16 performance, efficient; - _Static_assert(CONFIG(SOC_INTEL_COMMON_BLOCK_RUNTIME_CORE_SCALING_FACTORS) || - CONFIG_SOC_INTEL_PERFORMANCE_CORE_SCALE_FACTOR != 0, - "CONFIG_SOC_INTEL_PERFORMANCE_CORE_SCALE_FACTOR must not be zero"); - - _Static_assert(CONFIG(SOC_INTEL_COMMON_BLOCK_RUNTIME_CORE_SCALING_FACTORS) || - CONFIG_SOC_INTEL_EFFICIENT_CORE_SCALE_FACTOR != 0, - "CONFIG_SOC_INTEL_EFFICIENT_CORE_SCALE_FACTOR must not be zero"); - - if (!performance) { - if (CONFIG(SOC_INTEL_COMMON_BLOCK_RUNTIME_CORE_SCALING_FACTORS)) { - soc_read_core_scaling_factors(&performance, &efficient); - } else { - performance = CONFIG_SOC_INTEL_PERFORMANCE_CORE_SCALE_FACTOR; - efficient = CONFIG_SOC_INTEL_EFFICIENT_CORE_SCALE_FACTOR; - } - } + if (!performance) + soc_read_core_scaling_factors(&performance, &efficient); *perf_core_nom_perf = (u16)((max_non_turbo_ratio * performance) / 100); *eff_core_nom_perf = (u16)((max_non_turbo_ratio * efficient) / 100); diff --git a/src/soc/intel/common/block/include/intelblocks/acpi.h b/src/soc/intel/common/block/include/intelblocks/acpi.h index a3f8248..de81c37 100644 --- a/src/soc/intel/common/block/include/intelblocks/acpi.h +++ b/src/soc/intel/common/block/include/intelblocks/acpi.h @@ -26,12 +26,30 @@ unsigned long acpi_create_madt_lapics_with_nmis_hybrid(unsigned long current); +#if CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_CPU_HYBRID) /* * Read the performance and efficient core ratios. * This is to be implemented by the SoC specific code if * SOC_INTEL_COMMON_BLOCK_RUNTIME_CORE_SCALING_FACTORS is selected. */ +#if CONFIG(SOC_INTEL_COMMON_BLOCK_RUNTIME_CORE_SCALING_FACTORS) enum cb_err soc_read_core_scaling_factors(u16 *performance, u16 *efficient); +#else +static inline enum cb_err soc_read_core_scaling_factors(u16 *performance, u16 *efficient) +{ + _Static_assert(CONFIG_SOC_INTEL_PERFORMANCE_CORE_SCALE_FACTOR != 0, + "CONFIG_SOC_INTEL_PERFORMANCE_CORE_SCALE_FACTOR must not be zero"); + + _Static_assert(CONFIG_SOC_INTEL_EFFICIENT_CORE_SCALE_FACTOR != 0, + "CONFIG_SOC_INTEL_EFFICIENT_CORE_SCALE_FACTOR must not be zero"); + + *performance = CONFIG_SOC_INTEL_PERFORMANCE_CORE_SCALE_FACTOR; + *efficient = CONFIG_SOC_INTEL_EFFICIENT_CORE_SCALE_FACTOR; + + return CB_SUCCESS; +} +#endif +#endif /* SOC_INTEL_COMMON_BLOCK_ACPI_CPU_HYBRID */ /* Generates ACPI code to define _CPC control method */ void acpigen_write_CPPC_hybrid_method(int core_id); -- To view, visit https://review.coreboot.org/c/coreboot/+/85897?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I62e903bea07f2981dfcbaf61d3b918e7c332afc5 Gerrit-Change-Number: 85897 Gerrit-PatchSet: 3 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[XS] Change in coreboot[main]: payloads/external/Makefile.mk: Update linuxboot warning
by Maximilian Brune (Code Review) 13 Jan '25

13 Jan '25

Maximilian Brune has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/85957?usp=email ) Change subject: payloads/external/Makefile.mk: Update linuxboot warning ...................................................................... payloads/external/Makefile.mk: Update linuxboot warning Only print the warning if Linuxboot payload is actually selected, because we don't care otherwise. Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: I5008d685c52c1d4e0d7eba44c964c51a2a6f99c3 --- M payloads/external/Makefile.mk 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/85957/1 diff --git a/payloads/external/Makefile.mk b/payloads/external/Makefile.mk index 299f1ee..c279121 100644 --- a/payloads/external/Makefile.mk +++ b/payloads/external/Makefile.mk @@ -396,7 +396,10 @@ CONFIG_LINUXBOOT_CROSS_COMPILE_PATH=$(CROSS_COMPILE_$(LINUXBOOT_CROSS_COMPILE_ARCH-y)) endif # CONFIG_LINUXBOOT_CROSS_COMPILE_PATH else # CONFIG_LINUXBOOT_CROSS_COMPILE +ifeq ($(CONFIG_PAYLOAD_LINUXBOOT),y) + # only print warning when Linuxboot payload is actually selected $(warning "Using host toolchain to build Linuxboot") +endif # CONFIG_PAYLOAD_LINUXBOOT endif # CONFIG_LINUXBOOT_CROSS_COMPILE .PHONY: linuxboot payloads/external/LinuxBoot/build/Image payloads/external/LinuxBoot/build/initramfs linuxboot: -- To view, visit https://review.coreboot.org/c/coreboot/+/85957?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I5008d685c52c1d4e0d7eba44c964c51a2a6f99c3 Gerrit-Change-Number: 85957 Gerrit-PatchSet: 1 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com>

1 0

[M] Change in coreboot[main]: util/amdfwtool: Add binary parsing
by Maximilian Brune (Code Review) 13 Jan '25

13 Jan '25

Attention is currently required from: Ana Carolina Cabral, Anand Vaikar, Bao Zheng, Felix Held, Marshall Dawson, Maximilian Brune, Paul Menzel. Hello Anand Vaikar, Bao Zheng, Felix Held, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/84373?usp=email to look at the new patch set (#11). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: util/amdfwtool: Add binary parsing ...................................................................... util/amdfwtool: Add binary parsing This adds parsing for some more possible firmware blobs on AMD. These binaries are used on a mainboard based on glinda SOC. Tested: Boot birman_plus mainboard Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: I78d7a9dba71de557e0a9a885d8561eea1f4191ef Original-signed-off-by: Anand Vaikar <a.vaikar2021(a)gmail.com> --- M util/amdfwtool/amdfwtool.c M util/amdfwtool/amdfwtool.h M util/amdfwtool/data_parse.c 3 files changed, 105 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/84373/11 -- To view, visit https://review.coreboot.org/c/coreboot/+/84373?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I78d7a9dba71de557e0a9a885d8561eea1f4191ef Gerrit-Change-Number: 84373 Gerrit-PatchSet: 11 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Anand Vaikar <a.vaikar2021(a)gmail.com> Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Ana Carolina Cabral Gerrit-CC: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Anand Vaikar <a.vaikar2021(a)gmail.com> Gerrit-Attention: Ana Carolina Cabral Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>

1 0

[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add eDP driver
by Yu-Ping Wu (Code Review) 13 Jan '25

13 Jan '25

Attention is currently required from: Hung-Te Lin, Jarried Lin. Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85949?usp=email ) Change subject: soc/mediatek/mt8196: Add eDP driver ...................................................................... Patch Set 2: (50 comments) File src/soc/mediatek/mt8196/dptx.c: https://review.coreboot.org/c/coreboot/+/85949/comment/341b788c_506b9e09?us… : PS2, Line 51: swing > Please be consistent with the `mtk_edp_set_swing_pre_emphasis` argument names. […] Prefer `preemphasis` over `pre_emphasis`, as we already have the former name in src/soc/mediatek/common/dp/include/soc/dptx_hal_common.h. https://review.coreboot.org/c/coreboot/+/85949/comment/d16a7de8_759aa6ad?us… : PS2, Line 89: u8 target_link_rate = mtk_dp->train_info.linkrate; : u8 target_lane_count = mtk_dp->train_info.linklane_count; Should we pass the `linkrate` and `lanecount` from the for loop in `dptx_set_trainingstart()`, just like `dptx_trainingflow` in src/soc/mediatek/common/dp/dptx.c? Note that in the for loop, we reduce the local variable `linkrate` instead of `mtk_dp->train_info.linkrate`. That looks like a bug to me. https://review.coreboot.org/c/coreboot/+/85949/comment/f25a0c3a_3f39dde4?us… : PS2, Line 257: mtk_dp->train_info.linkrate = (linkrate >= mtk_dp->train_info.sys_max_linkrate) ? To simplify the data flow, can we set `mtk_dp->train_info` fields only at the end of this function (lines #334-335 below)? Before that, we can always use the local variables. https://review.coreboot.org/c/coreboot/+/85949/comment/51abc8bf_c9ef5893?us… : PS2, Line 257: (linkrate >= mtk_dp->train_info.sys_max_linkrate) ? : mtk_dp->train_info.sys_max_linkrate : linkrate; Use `MIN(...)` https://review.coreboot.org/c/coreboot/+/85949/comment/107199e8_213ba5ca?us… : PS2, Line 276: case DP_LINKRATE_HBR2: Does this not support DP_LINKRATE_HBR25? https://review.coreboot.org/c/coreboot/+/85949/comment/b5651dd2_90c4a805?us… : PS2, Line 300: -EIO Let's use `DPTX_TRANING_FAIL` for consistency. https://review.coreboot.org/c/coreboot/+/85949/comment/a9f048ab_8a30f3de?us… : PS2, Line 312: -EINVAL DPTX_TRANING_FAIL https://review.coreboot.org/c/coreboot/+/85949/comment/629f6323_b38d7604?us… : PS2, Line 315: } one blank line below https://review.coreboot.org/c/coreboot/+/85949/comment/5366cb50_b20ebfa7?us… : PS2, Line 318: ret DPTX_TRANING_FAIL https://review.coreboot.org/c/coreboot/+/85949/comment/6866897c_7e98256d?us… : PS2, Line 320: /* Reduce lane count */ : if (lanecount == 0) : return -EIO; : lanecount /= 2; Write `if (lanecount == DP_LANECOUNT_4) ...`. Please refer to src/soc/mediatek/common/dp/dptx.c. https://review.coreboot.org/c/coreboot/+/85949/comment/f283dcba_d9b3bc66?us… : PS2, Line 327: can run to this reach here https://review.coreboot.org/c/coreboot/+/85949/comment/5940fa47_9473a1f4?us… : PS2, Line 332: -ETIMEDOUT DPTX_TRANING_FAIL https://review.coreboot.org/c/coreboot/+/85949/comment/5463f012_51cd4804?us… : PS2, Line 345: 0 DPTX_PASS https://review.coreboot.org/c/coreboot/+/85949/comment/d41fd74d_159066a9?us… : PS2, Line 394: dptx_set_trainingstart(&mtk_edp); check return value File src/soc/mediatek/mt8196/dptx_hal.c: https://review.coreboot.org/c/coreboot/+/85949/comment/dd06ce90_9e9bb5a4?us… : PS2, Line 24: 0xff Upper case for all values https://review.coreboot.org/c/coreboot/+/85949/comment/bcc8f6d0_ddfece72?us… : PS2, Line 37: 0x60 `BIT(6) | BIT(5)` https://review.coreboot.org/c/coreboot/+/85949/comment/69609fa0_da371d7d?us… : PS2, Line 38: 10 << 2 Use hex https://review.coreboot.org/c/coreboot/+/85949/comment/8454dca4_2373fc75?us… : PS2, Line 52: switch (out_format) { ``` u32 val; switch (out_format) { case DP_COLOR_FORMAT_RGB_444: case DP_COLOR_FORMAT_YUV_444: val = 0; ... } mtk_dp_mask(mtk_dp, REG_303C_DP_ENCODER0_P0, val << 12, 0x7 << 12); ``` https://review.coreboot.org/c/coreboot/+/85949/comment/aec1e38a_b4f42005?us… : PS2, Line 63: default: If this is an error, print an error message and return. https://review.coreboot.org/c/coreboot/+/85949/comment/e3a7ac04_fa0a0fef?us… : PS2, Line 73: BIT(9) | BIT(8) 0x3 << 8 https://review.coreboot.org/c/coreboot/+/85949/comment/32704c05_2cd5c341?us… : PS2, Line 82: void mtk_edp_phy_set_swing_pre_emphasis(struct mtk_dp *mtk_dp, u8 lane_count, Can be static. https://review.coreboot.org/c/coreboot/+/85949/comment/ad0982ef_d740d75c?us… : PS2, Line 85: if (lane_count >= DPTX_LANE_MAX) { Change to assert. https://review.coreboot.org/c/coreboot/+/85949/comment/01483db9_dad5a502?us… : PS2, Line 89: && i < DPTX_LANE_MAX Remove. https://review.coreboot.org/c/coreboot/+/85949/comment/101cd09b_41357ed4?us… : PS2, Line 94: lane_count i https://review.coreboot.org/c/coreboot/+/85949/comment/3835bd11_8a8a2e7d?us… : PS2, Line 98: extra blank line https://review.coreboot.org/c/coreboot/+/85949/comment/15f356a6_747f8ca2?us… : PS2, Line 99: int Wrong type. Please upload a separate patch to fix this and preemphasis. https://review.coreboot.org/c/coreboot/+/85949/comment/4cb534c3_25c3a633?us… : PS2, Line 102: if (lane_num >= DPTX_LANE_MAX) { : printk(BIOS_ERR, "invalid lane number: %d\n", lane_num); : return false; : } assert https://review.coreboot.org/c/coreboot/+/85949/comment/6c60b8d3_bc7d2259?us… : PS2, Line 107: i < DPTX_LANE_MAX remove https://review.coreboot.org/c/coreboot/+/85949/comment/4c4a92d5_54766a40?us… : PS2, Line 112: lane_num i https://review.coreboot.org/c/coreboot/+/85949/comment/d77c0c92_b25bf998?us… : PS2, Line 134: /* edp phy init*/ remove https://review.coreboot.org/c/coreboot/+/85949/comment/9394f031_6161a919?us… : PS2, Line 138: BIT(0) | BIT(1) | BIT(2), : BIT(0) | BIT(1) | BIT(2)); Use GENMASK https://review.coreboot.org/c/coreboot/+/85949/comment/7001422b_f3d073c9?us… : PS2, Line 146: switch (value) { ``` u32 val; switch (value) { case ...: val = ...; ... } mtk_dp_phy_write(mtk_dp, 0x143C, val); ``` https://review.coreboot.org/c/coreboot/+/85949/comment/a936e838_71b19b94?us… : PS2, Line 161: break return https://review.coreboot.org/c/coreboot/+/85949/comment/62886a3b_7cd3674a?us… : PS2, Line 205: 0x1fff upper case https://review.coreboot.org/c/coreboot/+/85949/comment/43a9105f_0a966e05?us… : PS2, Line 302: 0x0000000f, 0x0000000f 0xF, 0xF https://review.coreboot.org/c/coreboot/+/85949/comment/525e226c_5626f237?us… : PS2, Line 307: mtk_edp_hal_reset_swing_pre_emphasis Should we rename this to `dptx_hal_reset_swing_preemphasis`? This looks similar to `dptx_hal_reset_swing_preemphasis` in src/soc/mediatek/common/dp/dptx_hal.c. And can we rename the current `dptx_hal_reset_swing_preemphasis` in this file to `dptx_hal_phy_reset_swing_preemphasis` (does this name make sense)? https://review.coreboot.org/c/coreboot/+/85949/comment/433d750e_669675fc?us… : PS2, Line 327: printk(BIOS_DEBUG, "[eDPTX] DP_PHY_DIG_TX_CTL_0:%#x\n", val); remove this or the log in line #324 https://review.coreboot.org/c/coreboot/+/85949/comment/9f2aa064_35b2e189?us… : PS2, Line 346: do { : val = mtk_dp_phy_read(mtk_dp, 0x146c); : if ((val & mask) == mask) : break; : mdelay(100); : } while (retry--); ``` if (!retry(WAIT_AUX_READY_RETRY_TIMES, (mtk_dp_phy_read(...) & mask) == mask, mdelay(100))) { printk(BIOS_ERR, ...); } ``` https://review.coreboot.org/c/coreboot/+/85949/comment/37632be9_050802d9?us… : PS2, Line 369: if ((value << 2) <= UINT8_MAX) { use assert https://review.coreboot.org/c/coreboot/+/85949/comment/3030c7d6_62d430c0?us… : PS2, Line 380: for (int lane = 0; lane < lane_count; lane++) : printk(BIOS_INFO, : "Link training lane%d: swing_val = 0x%x, pre-emphasis = 0x%x\n", lane, : swing_val[lane], pre_emphasis[lane]); Remove. These values are already printed in `mtk_edp_phy_set_swing_pre_emphasis`. https://review.coreboot.org/c/coreboot/+/85949/comment/42030be5_b90ead38?us… : PS2, Line 408: Set idle pattern failed, %s: Unexpected https://review.coreboot.org/c/coreboot/+/85949/comment/e1c8efcd_cee4b1a2?us… : PS2, Line 408: `__func__` File src/soc/mediatek/mt8196/include/soc/dp_intf.h: https://review.coreboot.org/c/coreboot/+/85949/comment/5b770f7a_4aafb715?us… : PS2, Line 199: gs_level Remove if unused https://review.coreboot.org/c/coreboot/+/85949/comment/5756d87c_7c2ca6cc?us… : PS2, Line 206: shift_half_line Remove if unused File src/soc/mediatek/mt8196/include/soc/dptx.h: https://review.coreboot.org/c/coreboot/+/85949/comment/96c85869_b901729b?us… : PS2, Line 6: #include <soc/dptx_common.h> one blank line below https://review.coreboot.org/c/coreboot/+/85949/comment/9bb6de46_86d173be?us… : PS2, Line 7: #define DP_LANSE_ADJUST_SIZE 2 Move to src/soc/mediatek/common/dp/include/soc/dptx_common.h, as the `dptx_train_tps1` function in src/soc/mediatek/common/dp/dptx.c can also use this value in a future follow-up patch. File src/soc/mediatek/mt8196/include/soc/dptx_hal.h: https://review.coreboot.org/c/coreboot/+/85949/comment/4c73fcfd_64acb490?us… : PS2, Line 26: enum { : ENODEV = 1, : ETIMEDOUT, : EIO, : EINVAL, : }; Remove these, and use `DPTX_*` values defined in src/soc/mediatek/common/dp/include/soc/dptx_common.h https://review.coreboot.org/c/coreboot/+/85949/comment/a576109b_6be83c9a?us… : PS2, Line 36: void mtk_edp_phy_set_swing_pre_emphasis(struct mtk_dp *mtk_dp, u8 lane_count, Remove here and change to static. https://review.coreboot.org/c/coreboot/+/85949/comment/4137ae6c_84039990?us… : PS2, Line 38: void mtk_edp_phy_wait_aux_ldo_ready(struct mtk_dp *mtk_dp); : void mtk_edp_hal_phy_set_idle_pattern(struct mtk_dp *mtk_dp, u8 lane_count, bool enable); : void mtk_edp_set_swing_pre_emphasis Ideally APIs in this file should start with `dptx_hal_`. Can we rename these? https://review.coreboot.org/c/coreboot/+/85949/comment/78053605_83860752?us… : PS2, Line 42: #endif /* __SOC_MEDIATEK_MT8196_DP_DPTX_HAL_H__ */ Add blank line above -- To view, visit https://review.coreboot.org/c/coreboot/+/85949?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Iccba53f6c6181ca84624c216f9641a2ae9041671 Gerrit-Change-Number: 85949 Gerrit-PatchSet: 2 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Mon, 13 Jan 2025 16:53:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>

1 0

[L] Change in coreboot[main]: ec/starlabs/merlin: Remove unused variant directories
by Matt DeVillier (Code Review) 13 Jan '25

13 Jan '25

Attention is currently required from: Sean Rhodes. Matt DeVillier has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/85956?usp=email ) Change subject: ec/starlabs/merlin: Remove unused variant directories ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/85956?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ibfdd858c6460a1291d6a15bb7eccee9e9006adff Gerrit-Change-Number: 85956 Gerrit-PatchSet: 1 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Comment-Date: Mon, 13 Jan 2025 16:32:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[M] Change in coreboot[main]: soc/mediatek/mt8196: Initialize MCUPM
by Yu-Ping Wu (Code Review) 13 Jan '25

13 Jan '25

Attention is currently required from: Hung-Te Lin, Jarried Lin, agogo. Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85888?usp=email ) Change subject: soc/mediatek/mt8196: Initialize MCUPM ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/85888?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I223f245d384f32d54f6170a28b29573638f77296 Gerrit-Change-Number: 85888 Gerrit-PatchSet: 5 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: agogo <agogo.huang(a)mediatek.corp-partner.google.com> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: agogo <agogo.huang(a)mediatek.corp-partner.google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Mon, 13 Jan 2025 14:36:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2025