Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31634
Change subject: util/inteltool: Refactoring #2 ......................................................................
util/inteltool: Refactoring #2
Change-Id: Ie92a5af141a907251fc847d53c767984949d9312 Signed-off-by: Felix Singer migy@darmstadt.ccc.de --- M util/inteltool/Makefile M util/inteltool/gpio.h A util/inteltool/gpio_test.c A util/inteltool/gpio_test.h 4 files changed, 447 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31634/1
diff --git a/util/inteltool/Makefile b/util/inteltool/Makefile index d88063b..0097831 100644 --- a/util/inteltool/Makefile +++ b/util/inteltool/Makefile @@ -28,7 +28,7 @@ CPPFLAGS += -I$(top)/src/commonlib/include
OBJS = inteltool.o pcr.o cpu.o gpio.o gpio_groups.o rootcmplx.o powermgt.o \ - memory.o pcie.o amb.o ivy_memory.o spi.o gfx.o ahci.o \ + memory.o pcie.o amb.o ivy_memory.o spi.o gfx.o ahci.o gpio_test.o \
OS_ARCH = $(shell uname) ifeq ($(OS_ARCH), Darwin) diff --git a/util/inteltool/gpio.h b/util/inteltool/gpio.h index c89f393..161e9bb 100644 --- a/util/inteltool/gpio.h +++ b/util/inteltool/gpio.h @@ -17,6 +17,36 @@ #ifndef GPIO_H #define GPIO_H
+ +// Describes a native function +struct native_mode { + uint8_t mode; + char *name; +}; + +/* + * Describes a GPIO pad, which optionally can + * have one or more native functions and a default + * mode. If no default mode is set, + * default_mode to -1, else set it to the + * function number. + */ +struct gpio_pad { + const char *name; + uint8_t default_mode; + const struct native_mode *modes; + uint64_t default_config; + uint64_t config; +}; + +/* +struct gpio_pad { + const char *name; + size_t defaultMode; + const char **functions; +}; +*/ + struct gpio_group { const char *display; size_t pad_count; diff --git a/util/inteltool/gpio_test.c b/util/inteltool/gpio_test.c new file mode 100644 index 0000000..9052623 --- /dev/null +++ b/util/inteltool/gpio_test.c @@ -0,0 +1,66 @@ +/* + * inteltool - dump all registers on an Intel CPU + chipset based system. + * + * Copyright (C) 2017 secunet Security Networks AG + * Copyright (C) 2019 Felix Singer migy@darmstadt.ccc.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <stdio.h> +#include <stdlib.h> +#include <stddef.h> +#include <stdint.h> +#include <assert.h> +#include <inttypes.h> +#include "inteltool.h" +#include "pcr.h" +#include "gpio.h" +#include "gpio_test.h" + +#define SBBAR_SIZE (16 * MiB) +#define PCR_PORT_SIZE (64 * KiB) + +void select_gpio_community(struct pci_dev *const sb) +{ + size_t community_count; + const struct gpio_community *const *communities; + + switch (sb->device_id) { + case PCI_DEVICE_ID_INTEL_B150: + case PCI_DEVICE_ID_INTEL_CM236: +// community_count = ARRAY_SIZE(sunrise_communities); +// communities = sunrise_communities; +// pcr_init(sb); + break; + case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_BASE: + case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_PREM: + case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_Y_PREM: + case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_IHDCP_BASE: + case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_IHDCP_PREM: + case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_Y_IHDCP_PREM: +// community_count = ARRAY_SIZE(sunrise_lp_communities); +// communities = sunrise_lp_communities; +// pcr_init(sb); + break; + case PCI_DEVICE_ID_INTEL_DNV_LPC: +// community_count = ARRAY_SIZE(denverton_communities); +// communities = denverton_communities; +// pcr_init(sb); + break; + default: + return; + } + + printf("\n============= GPIOS =============\n\n"); + +// for (; community_count; --community_count) +// print_gpio_community(*communities++); +} diff --git a/util/inteltool/gpio_test.h b/util/inteltool/gpio_test.h new file mode 100644 index 0000000..9edc4fe --- /dev/null +++ b/util/inteltool/gpio_test.h @@ -0,0 +1,350 @@ +/* + * inteltool - dump all registers on an Intel CPU + chipset based system. + * + * Copyright (C) 2017 secunet Security Networks AG + * Copyright (C) 2019 Felix Singer migy@darmstadt.ccc.de + * Copyright (C) 2019 by 9elements Agency GmbH + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include "gpio.h" + +#ifndef GPIO_TEST_H +#define GPIO_TEST_H + +/* +const struct gpio_pad test[] = { + { "gpio1", 2, (const char *[]) { "function1", "function2", NULL, } }, +}; +*/ + +const struct gpio_pad test[] = { + { "GPP_A0", -1, (const struct native_mode[]) { {1, "RCIN#" }, }, }, + { "GPP_A1", -1, (const struct native_mode[]) { {1, "LAD0" }, {3, "ESPI_IO0" }, }, }, + { "GPP_A2", -1, (const struct native_mode[]) { {1, "LAD1" }, {3, "ESPI_IO1" }, }, }, + { "GPP_A3", -1, (const struct native_mode[]) { {1, "LAD2" }, {3, "ESPI_IO2" }, }, }, + { "GPP_A4", -1, (const struct native_mode[]) { {1, "LAD3" }, {3, "ESPI_IO3" }, }, }, + { "GPP_A5", -1, (const struct native_mode[]) { {1, "LFRAME#" }, {3, "ESPI_CS#" }, }, }, + { "GPP_A6", -1, (const struct native_mode[]) { {1, "SERIRQ#" }, }, }, + { "GPP_A7", -1, (const struct native_mode[]) { {1, "PIRQA#" }, }, }, + { "GPP_A8", -1, (const struct native_mode[]) { {1, "CLKRUN#" }, }, }, + { "GPP_A9", -1, (const struct native_mode[]) { {1, "CLKOUT_LPC0" }, {3, "ESPI_CLK" }, }, }, + { "GPP_A10", -1, (const struct native_mode[]) { {1, "CLKOUT_LPC1" }, }, }, + { "GPP_A11", -1, (const struct native_mode[]) { {1, "PME#" }, }, }, + { "GPP_A12", -1, (const struct native_mode[]) { {1, "BM_BUSY#" }, {2, "ISH_GP6" }, {3, "SX_EXIT_HOLDOFF#" }, }, }, + { "GPP_A13", -1, (const struct native_mode[]) { {1, "SUSWARN#/SUSPWRDNACK" }, }, }, + { "GPP_A14", -1, (const struct native_mode[]) { {1, "SUS_STAT#" }, {3, "ESPI_RESET#" }, }, }, + { "GPP_A15", -1, (const struct native_mode[]) { {1, "SUS_ACK#" }, }, }, + { "GPP_A16", -1, (const struct native_mode[]) { {1, "SD_1P8_SEL" }, }, }, + { "GPP_A17", -1, (const struct native_mode[]) { {1, "SD_PWR_EN#" }, {2, "ISH_GP7" }, }, }, + { "GPP_A18", -1, (const struct native_mode[]) { {1, "ISH_GP0" }, }, }, + { "GPP_A19", -1, (const struct native_mode[]) { {1, "ISH_GP1" }, }, }, + { "GPP_A20", -1, (const struct native_mode[]) { {1, "ISH_GP2" }, }, }, + { "GPP_A21", -1, (const struct native_mode[]) { {1, "ISH_GP3" }, }, }, + { "GPP_A22", -1, (const struct native_mode[]) { {1, "ISH_GP4" }, }, }, + { "GPP_A23", -1, (const struct native_mode[]) { {1, "ISH_GP5" }, }, }, +}; + +/* +const char *const sunrise_lp_group_a_names[] = { + "GPP_A0", "RCIN#", "n/a", "n/a", + "GPP_A1", "LAD0", "n/a", "ESPI_IO0", + "GPP_A2", "LAD1", "n/a", "ESPI_IO1", + "GPP_A3", "LAD2", "n/a", "ESPI_IO2", + "GPP_A4", "LAD3", "n/a", "ESPI_IO3", + "GPP_A5", "LFRAME#", "n/a", "ESPI_CS#", + "GPP_A6", "SERIRQ", "n/a", "n/a", + "GPP_A7", "PIRQA#", "n/a", "n/a", + "GPP_A8", "CLKRUN#", "n/a", "n/a", + "GPP_A9", "CLKOUT_LPC0", "n/a", "ESPI_CLK", + "GPP_A10", "CLKOUT_LPC1", "n/a", "n/a", + "GPP_A11", "PME#", "n/a", "n/a", + "GPP_A12", "BM_BUSY#", "ISH_GP6", "SX_EXIT_HOLDOFF#", + "GPP_A13", "SUSWARN#/SUSPWRDNACK", "n/a", "n/a", + "GPP_A14", "SUS_STAT#", "n/a", "ESPI_RESET#", + "GPP_A15", "SUS_ACK#", "n/a", "n/a", + "GPP_A16", "SD_1P8_SEL", "n/a", "n/a", + "GPP_A17", "SD_PWR_EN#", "ISH_GP7", "n/a", + "GPP_A18", "ISH_GP0", "n/a", "n/a", + "GPP_A19", "ISH_GP1", "n/a", "n/a", + "GPP_A20", "ISH_GP2", "n/a", "n/a", + "GPP_A21", "ISH_GP3", "n/a", "n/a", + "GPP_A22", "ISH_GP4", "n/a", "n/a", + "GPP_A23", "ISH_GP5", "n/a", "n/a", +}; + +const struct gpio_group sunrise_lp_group_a = { + .display = "------- GPIO group GPP_A -------", + .pad_count = ARRAY_SIZE(sunrise_lp_group_a_names) / 4, + .func_count = 4, + .pad_names = sunrise_lp_group_a_names, +}; + +const char *const sunrise_lp_group_b_names[] = { + "GPP_B0", "CORE_VID0", "n/a", "n/a", + "GPP_B1", "CORE_VID1", "n/a", "n/a", + "GPP_B2", "VRALERT#", "n/a", "n/a", + "GPP_B3", "CPU_GP2", "n/a", "n/a", + "GPP_B4", "CPU_GP3", "n/a", "n/a", + "GPP_B5", "SRCCLKREQ0#", "n/a", "n/a", + "GPP_B6", "SRCCLKREQ1#", "n/a", "n/a", + "GPP_B7", "SRCCLKREQ2#", "n/a", "n/a", + "GPP_B8", "SRCCLKREQ3#", "n/a", "n/a", + "GPP_B9", "SRCCLKREQ4#", "n/a", "n/a", + "GPP_B10", "SRCCLKREQ5#", "n/a", "n/a", + "GPP_B11", "EXT_PWR_GATE#", "n/a", "n/a", + "GPP_B12", "SLP_S0#", "n/a", "n/a", + "GPP_B13", "PLTRST#", "n/a", "n/a", + "GPP_B14", "SPKR", "n/a", "n/a", + "GPP_B15", "GSPI0_CS#", "n/a", "n/a", + "GPP_B16", "GSPI0_CLK", "n/a", "n/a", + "GPP_B17", "GSPI0_MISO", "n/a", "n/a", + "GPP_B18", "GSPI0_MOSI", "n/a", "n/a", + "GPP_B19", "GSPI1_CS#", "n/a", "n/a", + "GPP_B20", "GSPI1_CLK", "n/a", "n/a", + "GPP_B21", "GSPI1_MISO", "n/a", "n/a", + "GPP_B22", "GSPI1_MOSI", "n/a", "n/a", + "GPP_B23", "SML1ALERT#", "PCHHOT#", "n/a", +}; + +const struct gpio_group sunrise_lp_group_b = { + .display = "------- GPIO Group GPP_B -------", + .pad_count = ARRAY_SIZE(sunrise_lp_group_b_names) / 4, + .func_count = 4, + .pad_names = sunrise_lp_group_b_names, +}; + +const struct gpio_group *const sunrise_lp_community_ab_groups[] = { + &sunrise_lp_group_a, &sunrise_lp_group_b, +}; + +const struct gpio_community sunrise_lp_community_ab = { + .name = "------- GPIO Community 0 -------", + .pcr_port_id = 0xaf, + .group_count = ARRAY_SIZE(sunrise_lp_community_ab_groups), + .groups = sunrise_lp_community_ab_groups, +}; + +const char *const sunrise_group_c_names[] = { + "GPP_C0", "SMBCLK", "n/a", "n/a", + "GPP_C1", "SMBDATA", "n/a", "n/a", + "GPP_C2", "SMBALERT#", "n/a", "n/a", + "GPP_C3", "SML0CLK", "n/a", "n/a", + "GPP_C4", "SML0DATA", "n/a", "n/a", + "GPP_C5", "SML0ALERT#", "n/a", "n/a", + "GPP_C6", "SML1CLK", "n/a", "n/a", + "GPP_C7", "SML1DATA", "n/a", "n/a", + "GPP_C8", "UART0_RXD", "n/a", "n/a", + "GPP_C9", "UART0_TXD", "n/a", "n/a", + "GPP_C10", "UART0_RTS#", "n/a", "n/a", + "GPP_C11", "UART0_CTS#", "n/a", "n/a", + "GPP_C12", "UART1_RXD", "ISH_UART1_RXD", "n/a", + "GPP_C13", "UART1_TXD", "ISH_UART1_TXD", "n/a", + "GPP_C14", "UART1_RTS#", "ISH_UART1_RTS#", "n/a", + "GPP_C15", "UART1_CTS#", "ISH_UART1_CTS#", "n/a", + "GPP_C16", "I2C0_SDA", "n/a", "n/a", + "GPP_C17", "I2C0_SCL", "n/a", "n/a", + "GPP_C18", "I2C1_SDA", "n/a", "n/a", + "GPP_C19", "I2C1_SCL", "n/a", "n/a", + "GPP_C20", "UART2_RXD", "n/a", "n/a", + "GPP_C21", "UART2_TXD", "n/a", "n/a", + "GPP_C22", "UART2_RTS#", "n/a", "n/a", + "GPP_C23", "UART2_CTS#", "n/a", "n/a", +}; + +const struct gpio_group sunrise_group_c = { + .display = "------- GPIO Group GPP_C -------", + .pad_count = ARRAY_SIZE(sunrise_group_c_names) / 4, + .func_count = 4, + .pad_names = sunrise_group_c_names, +}; + +const char *const sunrise_lp_group_d_names[] = { + "GPP_D0", "SPI1_CS#", "n/a", "n/a", + "GPP_D1", "SPI1_CLK", "n/a", "n/a", + "GPP_D2", "SPI1_MISO", "n/a", "n/a", + "GPP_D3", "SPI1_MOSI", "n/a", "n/a", + "GPP_D4", "FLASHTRIG", "n/a", "n/a", + "GPP_D5", "ISH_I2C0_SDA", "n/a", "n/a", + "GPP_D6", "ISH_I2C0_SCL", "n/a", "n/a", + "GPP_D7", "ISH_I2C1_SDA", "n/a", "n/a", + "GPP_D8", "ISH_I2C1_SCL", "n/a", "n/a", + "GPP_D9", "n/a", "n/a", "n/a", + "GPP_D10", "n/a", "n/a", "n/a", + "GPP_D11", "n/a", "n/a", "n/a", + "GPP_D12", "n/a", "n/a", "n/a", + "GPP_D13", "ISH_UART0_RXD", "n/a", "n/a", + "GPP_D14", "ISH_UART0_TXD", "n/a", "n/a", + "GPP_D15", "ISH_UART0_RTS#", "n/a", "n/a", + "GPP_D16", "ISH_UART0_CTS#", "n/a", "n/a", + "GPP_D17", "DMIC_CLK1", "n/a", "n/a", + "GPP_D18", "DMIC_DATA1", "n/a", "n/a", + "GPP_D19", "DMIC_CLK0", "n/a", "n/a", + "GPP_D20", "DMIC_DATA0", "n/a", "n/a", + "GPP_D21", "SPI1_IO2", "n/a", "n/a", + "GPP_D22", "SPI1_IO3", "n/a", "n/a", + "GPP_D23", "I2S_MCLK", "n/a", "n/a", +}; + +const struct gpio_group sunrise_lp_group_d = { + .display = "------- GPIO Group GPP_D -------", + .pad_count = ARRAY_SIZE(sunrise_lp_group_d_names) / 4, + .func_count = 4, + .pad_names = sunrise_lp_group_d_names, +}; + +const char *const sunrise_lp_group_e_names[] = { + "GPP_E0", "SATAXPCIE0", "SATAGP0", "n/a", + "GPP_E1", "SATAXPCIE1", "SATAGP1", "n/a", + "GPP_E2", "SATAXPCIE2", "SATAGP2", "n/a", + "GPP_E3", "CPU_GP0", "n/a", "n/a", + "GPP_E4", "SATA_DEVSLP0", "n/a", "n/a", + "GPP_E5", "SATA_DEVSLP1", "n/a", "n/a", + "GPP_E6", "SATA_DEVSLP2", "n/a", "n/a", + "GPP_E7", "CPU_GP1", "n/a", "n/a", + "GPP_E8", "SATALED#", "n/a", "n/a", + "GPP_E9", "USB2_OC0#", "n/a", "n/a", + "GPP_E10", "USB2_OC1#", "n/a", "n/a", + "GPP_E11", "USB2_OC2#", "n/a", "n/a", + "GPP_E12", "USB2_OC3#", "n/a", "n/a", + "GPP_E13", "DDPB_HPD0", "n/a", "n/a", + "GPP_E14", "DDPC_HPD1", "n/a", "n/a", + "GPP_E15", "DDPD_HPD2", "n/a", "n/a", + "GPP_E16", "DDPE_HPD3", "n/a", "n/a", + "GPP_E17", "EDP_HPD", "n/a", "n/a", + "GPP_E18", "DDPB_CTRLCLK", "n/a", "n/a", + "GPP_E19", "DDPB_CTRLDATA", "n/a", "n/a", + "GPP_E20", "DDPC_CTRLCLK", "n/a", "n/a", + "GPP_E21", "DDPC_CTRLDATA", "n/a", "n/a", + "GPP_E22", "n/a", "n/a", "n/a", + "GPP_E23", "n/a", "n/a", "n/a", +}; + +const struct gpio_group sunrise_lp_group_e = { + .display = "------- GPIO Group GPP_E -------", + .pad_count = ARRAY_SIZE(sunrise_lp_group_e_names) / 4, + .func_count = 4, + .pad_names = sunrise_lp_group_e_names, +}; + +const char *const sunrise_lp_group_f_names[] = { + "GPP_F0", "I2S2_SCLK", "n/a", "n/a", + "GPP_F1", "I2S2_SFRM", "n/a", "n/a", + "GPP_F2", "I2S2_TXD", "n/a", "n/a", + "GPP_F3", "I2S2_RXD", "n/a", "n/a", + "GPP_F4", "I2C2_SDA", "n/a", "n/a", + "GPP_F5", "I2C2_SCL", "n/a", "n/a", + "GPP_F6", "I2C3_SDA", "n/a", "n/a", + "GPP_F7", "I2C3_SCL", "n/a", "n/a", + "GPP_F8", "I2C4_SDA", "n/a", "n/a", + "GPP_F9", "I2C4_SCL", "n/a", "n/a", + "GPP_F10", "I2C5_SDA", "ISH_I2C2_SDA", "n/a", + "GPP_F11", "I2C5_SCL", "ISH_I2C2_SCL", "n/a", + "GPP_F12", "EMMC_CMD", "n/a", "n/a", + "GPP_F13", "EMMC_DATA0", "n/a", "n/a", + "GPP_F14", "EMMC_DATA1", "n/a", "n/a", + "GPP_F15", "EMMC_DATA2", "n/a", "n/a", + "GPP_F16", "EMMC_DATA3", "n/a", "n/a", + "GPP_F17", "EMMC_DATA4", "n/a", "n/a", + "GPP_F18", "EMMC_DATA5", "n/a", "n/a", + "GPP_F19", "EMMC_DATA6", "n/a", "n/a", + "GPP_F20", "EMMC_DATA7", "n/a", "n/a", + "GPP_F21", "EMMC_RCLK", "n/a", "n/a", + "GPP_F22", "EMMC_CLK", "n/a", "n/a", + "GPP_F23", "n/a", "n/a", "n/a", +}; + +const struct gpio_group sunrise_lp_group_f = { + .display = "------- GPIO Group GPP_F -------", + .pad_count = ARRAY_SIZE(sunrise_lp_group_f_names) / 4, + .func_count = 4, + .pad_names = sunrise_lp_group_f_names, +}; + +const char *const sunrise_lp_group_g_names[] = { + "GPP_G0", "SD_CMD", "n/a", "n/a", + "GPP_G1", "SD_DATA0", "n/a", "n/a", + "GPP_G2", "SD_DATA1", "n/a", "n/a", + "GPP_G3", "SD_DATA2", "n/a", "n/a", + "GPP_G4", "SD_DATA3", "n/a", "n/a", + "GPP_G5", "SD_CD#", "n/a", "n/a", + "GPP_G6", "SD_CLK", "n/a", "n/a", + "GPP_G7", "SD_WP", "n/a", "n/a", +}; + +const struct gpio_group sunrise_lp_group_g = { + .display = "------- GPIO Group GPP_G -------", + .pad_count = ARRAY_SIZE(sunrise_lp_group_g_names) / 4, + .func_count = 4, + .pad_names = sunrise_lp_group_g_names, +}; + +const struct gpio_group *const sunrise_lp_community_cde_groups[] = { + &sunrise_group_c, &sunrise_lp_group_d, &sunrise_lp_group_e, +}; + +const struct gpio_community sunrise_lp_community_cde = { + .name = "------- GPIO Community 1 -------", + .pcr_port_id = 0xae, + .group_count = ARRAY_SIZE(sunrise_lp_community_cde_groups), + .groups = sunrise_lp_community_cde_groups, +}; + +const char *const sunrise_group_gpd_names[] = { + "GPD0", "BATLOW#", "n/a", "n/a", + "GPD1", "ACPRESENT", "n/a", "n/a", + "GPD2", "LAN_WAKE#", "n/a", "n/a", + "GPD3", "PWRBTN#", "n/a", "n/a", + "GPD4", "SLP_S3#", "n/a", "n/a", + "GPD5", "SLP_S4#", "n/a", "n/a", + "GPD6", "SLP_A#", "n/a", "n/a", + "GPD7", "RESERVED", "n/a", "n/a", + "GPD8", "SUSCLK", "n/a", "n/a", + "GPD9", "SLP_WLAN#", "n/a", "n/a", + "GPD10", "SLP_S5#", "n/a", "n/a", + "GPD11", "LANPHYPC", "n/a", "n/a", +}; + +const struct gpio_group sunrise_group_gpd = { + .display = "-------- GPIO Group GPD --------", + .pad_count = ARRAY_SIZE(sunrise_group_gpd_names) / 4, + .func_count = 4, + .pad_names = sunrise_group_gpd_names, +}; + +const struct gpio_group *const sunrise_community_gpd_groups[] = { + &sunrise_group_gpd, +}; + +const struct gpio_community sunrise_community_gpd = { + .name = "------- GPIO Community 2 -------", + .pcr_port_id = 0xad, + .group_count = ARRAY_SIZE(sunrise_community_gpd_groups), + .groups = sunrise_community_gpd_groups, +}; + +const struct gpio_group *const sunrise_lp_community_fg_groups[] = { + &sunrise_lp_group_f, &sunrise_lp_group_g, +}; + +const struct gpio_community sunrise_lp_community_fg = { + .name = "------- GPIO Community 3 -------", + .pcr_port_id = 0xac, + .group_count = ARRAY_SIZE(sunrise_lp_community_fg_groups), + .groups = sunrise_lp_community_fg_groups, +}; + +const struct gpio_community *const sunrise_lp_communities[] = { + &sunrise_lp_community_ab, &sunrise_lp_community_cde, + &sunrise_community_gpd, &sunrise_lp_community_fg, +}; +*/ +#endif
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 10:
This change is ready for review.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h File util/inteltool/gpio.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h@34 PS10, Line 34: * Modes: : * -2 = GP-Out : * -1 = GP-In : * 0 = Undefined : * >0 = Native functions this should be an `enum`.
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h@44 PS10, Line 44: uint32_t config0; : uint32_t config1; So these would be used to cache the register content for later processing? or for default values?
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h File util/inteltool/gpio_test.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h@24 PS10, Line 24: .name What do we gain from repeating the field identifiers in each row? IMO, it only wastes space.
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h@26 PS10, Line 26: FUNC(3, "ESPI_IO0") } }, Personally, I prefer more condensed tables where a row fits a single line. It feels easier to review. But that's just my opinion. Let's hear others, too.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h File util/inteltool/gpio.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h@34 PS10, Line 34: * Modes: : * -2 = GP-Out : * -1 = GP-In : * 0 = Undefined : * >0 = Native functions
this should be an `enum`.
Ack
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h@44 PS10, Line 44: uint32_t config0; : uint32_t config1;
So these would be used to cache the register content for later processing? […]
These are meant as cache for the register contents. I also thought about storing the default values (ex. for comparison), but according to the datasheets (ex. Kabylake) some registers have values like this 44000x00h. I think x is a placeholder, but I have no idea what that means.
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h File util/inteltool/gpio_test.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h@24 PS10, Line 24: .name
What do we gain from repeating the field identifiers in each […]
I think the brackets make it more messy. Using the field identifiers I tried to improve the overview over the columns.
We also can use spaces/tabs.
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h@26 PS10, Line 26: FUNC(3, "ESPI_IO0") } },
Personally, I prefer more condensed tables where a row fits a single line. […]
I am fine with both.
Marius Genheimer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h File util/inteltool/gpio_test.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h@26 PS10, Line 26: FUNC(3, "ESPI_IO0") } },
I am fine with both.
I'm fine with both display styles as well, but I think in terms of readability the multi-line version has an edge over the single-line approach.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 11:
I added some thoughts to the source code. Would like to know your opinions.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h File util/inteltool/gpio.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h@34 PS10, Line 34: * Modes: : * -2 = GP-Out : * -1 = GP-In : * 0 = Undefined : * >0 = Native functions
Ack
Done
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 12:
This change is ready for review.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/#/c/31634/12/util/inteltool/gpio.h File util/inteltool/gpio.h:
https://review.coreboot.org/#/c/31634/12/util/inteltool/gpio.h@60 PS12, Line 60: uint32_t config[2]; Please move into a separate commit along with the intended code changes.
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h File util/inteltool/gpio_test.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h@24 PS10, Line 24: .name
I think the brackets make it more messy. […]
Most pads would fit a single line (increase readabitity, IMHO) if we'd leave out the identifiers. e.g.
{ "GPP_A1", GPIO, { { 1, "LAD0" }, { 3, "ESPI_IO0" } } },
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h File util/inteltool/gpio.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h@44 PS10, Line 44: uint32_t config0; : uint32_t config1;
These are meant as cache for the register contents. […]
`x` means the default value depends on more than the platform (e.g. pin straps, soft straps in the IFD). It makes handling of default values very tedious. Hard to tell if it would be worth the effort to add them.
Felix Singer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Abandoned
-
Felix Singer (Code Review) -
Marius Genheimer (Code Review)
-
Nico Huber (Code Review)