[coreboot-gerrit] New patch to review for coreboot: soc/intel/common/lpss_i2c: simplify API and use common config structure

Aaron Durbin (adurbin@chromium.org) gerrit at coreboot.org
Thu Nov 10 00:18:39 CET 2016


Aaron Durbin (adurbin at chromium.org) just uploaded a new patch set to gerrit, which you can find at https://review.coreboot.org/17348

-gerrit

commit 63d6d83f54429f634d27aa02887038fa4153c566
Author: Aaron Durbin <adurbin at chromium.org>
Date:   Wed Nov 9 17:09:40 2016 -0600

    soc/intel/common/lpss_i2c: simplify API and use common config structure
    
    The apollolake and skylake had dupcliate stanzas of code for
    initializing the i2c buses. Additionally, they also had very
    similar structures for providing settings for the i2c speed
    control. Introduce a new struct lpss_i2c_bus_config and
    utilize it in both apollolake and skylake thereby removing
    the need for SoC-specific structres. The new structure is
    used for initializing a bus fully as the lpss i2c API is
    simplified in that lpss_i2c_init() is only required to be
    called. The struct lpss_i2c_bus_config structure is passed
    in for both initializing and filling in the SSDT information.
    The formerly exposed functions are made static to reduce the
    external API exposure.
    
    BUG=chrome-os-partner:58889
    
    Change-Id: Ib4fa8a7a4de052da75c778a7658741a5a8e0e6b9
    Signed-off-by: Aaron Durbin <adurbin at chromium.org>
---
 src/soc/intel/apollolake/chip.h       | 11 +----
 src/soc/intel/apollolake/i2c.c        | 18 ++-----
 src/soc/intel/apollolake/i2c_early.c  | 14 +-----
 src/soc/intel/common/lpss_i2c.c       | 90 +++++++++++++++++++----------------
 src/soc/intel/common/lpss_i2c.h       | 40 +++++-----------
 src/soc/intel/skylake/bootblock/i2c.c | 14 +-----
 src/soc/intel/skylake/chip.h          |  2 +-
 src/soc/intel/skylake/i2c.c           | 18 ++-----
 8 files changed, 70 insertions(+), 137 deletions(-)

diff --git a/src/soc/intel/apollolake/chip.h b/src/soc/intel/apollolake/chip.h
index 6c3bcd8..dd30106 100644
--- a/src/soc/intel/apollolake/chip.h
+++ b/src/soc/intel/apollolake/chip.h
@@ -28,15 +28,6 @@
 #define CLKREQ_DISABLED		0xf
 #define APOLLOLAKE_I2C_DEV_MAX	8
 
-struct apollolake_i2c_config {
-	/* Bus should be enabled prior to ramstage with temporary base */
-	int early_init;
-	/* Bus speed in Hz, default is I2C_SPEED_FAST (400 KHz) */
-	enum i2c_speed speed;
-	/* Specific bus speed configuration */
-	struct lpss_i2c_speed_config speed_config[LPSS_I2C_SPEED_CONFIG_COUNT];
-};
-
 /* Serial IRQ control. SERIRQ_QUIET is the default (0). */
 enum serirq_mode {
 	SERIRQ_QUIET,
@@ -95,7 +86,7 @@ struct soc_intel_apollolake_config {
 	enum serirq_mode serirq_mode;
 
 	/* I2C bus configuration */
-	struct apollolake_i2c_config i2c[APOLLOLAKE_I2C_DEV_MAX];
+	struct lpss_i2c_bus_config i2c[APOLLOLAKE_I2C_DEV_MAX];
 
 	uint8_t gpe0_dw1; /* GPE0_63_32 STS/EN */
 	uint8_t gpe0_dw2; /* GPE0_95_64 STS/EN */
diff --git a/src/soc/intel/apollolake/i2c.c b/src/soc/intel/apollolake/i2c.c
index 1c16a06..69cb4b8 100644
--- a/src/soc/intel/apollolake/i2c.c
+++ b/src/soc/intel/apollolake/i2c.c
@@ -60,24 +60,12 @@ static int i2c_dev_to_bus(struct device *dev)
 static void i2c_dev_init(struct device *dev)
 {
 	struct soc_intel_apollolake_config *config = dev->chip_info;
-	const struct lpss_i2c_speed_config *sptr;
-	enum i2c_speed speed;
-	int i, bus = i2c_dev_to_bus(dev);
+	int bus = i2c_dev_to_bus(dev);
 
 	if (!config || bus < 0)
 		return;
 
-	speed = config->i2c[bus].speed ? : I2C_SPEED_FAST;
-	lpss_i2c_init(bus, speed);
-
-	/* Apply custom speed config if it has been set by the board */
-	for (i = 0; i < LPSS_I2C_SPEED_CONFIG_COUNT; i++) {
-		sptr = &config->i2c[bus].speed_config[i];
-		if (sptr->speed == speed) {
-			lpss_i2c_set_speed_config(bus, sptr);
-			break;
-		}
-	}
+	lpss_i2c_init(bus, &config->i2c[bus]);
 }
 
 static void i2c_fill_ssdt(struct device *dev)
@@ -89,7 +77,7 @@ static void i2c_fill_ssdt(struct device *dev)
 		return;
 
 	acpigen_write_scope(acpi_device_path(dev));
-	lpss_i2c_acpi_fill_ssdt(config->i2c[bus].speed_config);
+	lpss_i2c_acpi_fill_ssdt(&config->i2c[bus]);
 	acpigen_pop_len();
 }
 
diff --git a/src/soc/intel/apollolake/i2c_early.c b/src/soc/intel/apollolake/i2c_early.c
index 82883f8..7c188a8 100644
--- a/src/soc/intel/apollolake/i2c_early.c
+++ b/src/soc/intel/apollolake/i2c_early.c
@@ -29,8 +29,6 @@ static int i2c_early_init_bus(unsigned bus)
 {
 	ROMSTAGE_CONST struct soc_intel_apollolake_config *config;
 	ROMSTAGE_CONST struct device *tree_dev;
-	const struct lpss_i2c_speed_config *sptr;
-	enum i2c_speed speed;
 	pci_devfn_t dev;
 	int devfn;
 	uintptr_t base;
@@ -72,21 +70,11 @@ static int i2c_early_init_bus(unsigned bus)
 	write32(reg, value);
 
 	/* Initialize the controller */
-	speed = config->i2c[bus].speed ? : I2C_SPEED_FAST;
-	if (lpss_i2c_init(bus, speed) < 0) {
+	if (lpss_i2c_init(bus, &config->i2c[bus]) < 0) {
 		printk(BIOS_ERR, "I2C%u failed to initialize\n", bus);
 		return -1;
 	}
 
-	/* Apply custom speed config if it has been set by the board */
-	for (value = 0; value < LPSS_I2C_SPEED_CONFIG_COUNT; value++) {
-		sptr = &config->i2c[bus].speed_config[value];
-		if (sptr->speed == speed) {
-			lpss_i2c_set_speed_config(bus, sptr);
-			break;
-		}
-	}
-
 	return 0;
 }
 
diff --git a/src/soc/intel/common/lpss_i2c.c b/src/soc/intel/common/lpss_i2c.c
index 58d44b8..6fe8f10 100644
--- a/src/soc/intel/common/lpss_i2c.c
+++ b/src/soc/intel/common/lpss_i2c.c
@@ -370,39 +370,8 @@ static void lpss_i2c_acpi_write_speed_config(
 	acpigen_pop_len();
 }
 
-void lpss_i2c_acpi_fill_ssdt(const struct lpss_i2c_speed_config *override)
-{
-	const struct lpss_i2c_speed_config *sptr;
-	struct lpss_i2c_speed_config sgen;
-	enum i2c_speed speeds[LPSS_I2C_SPEED_CONFIG_COUNT] = {
-		I2C_SPEED_STANDARD,
-		I2C_SPEED_FAST,
-		I2C_SPEED_FAST_PLUS,
-		I2C_SPEED_HIGH,
-	};
-	int i;
-
-	/* Report timing values for the OS driver */
-	for (i = 0; i < LPSS_I2C_SPEED_CONFIG_COUNT; i++) {
-		/* Generate speed config for default case */
-		if (lpss_i2c_gen_speed_config(speeds[i], &sgen) < 0)
-			continue;
-
-		/* Apply board specific override for this speed if found */
-		for (sptr = override; sptr && sptr->speed; sptr++) {
-			if (sptr->speed == speeds[i]) {
-				memcpy(&sgen, sptr, sizeof(sgen));
-				break;
-			}
-		}
-
-		/* Generate ACPI based on selected speed config */
-		lpss_i2c_acpi_write_speed_config(&sgen);
-	}
-}
-
-int lpss_i2c_set_speed_config(unsigned bus,
-			      const struct lpss_i2c_speed_config *config)
+static int lpss_i2c_set_speed_config(unsigned bus,
+				const struct lpss_i2c_speed_config *config)
 {
 	struct lpss_i2c_regs *regs;
 	void *hcnt_reg, *lcnt_reg;
@@ -442,16 +411,26 @@ int lpss_i2c_set_speed_config(unsigned bus,
 	return 0;
 }
 
-int lpss_i2c_gen_speed_config(enum i2c_speed speed,
-			      struct lpss_i2c_speed_config *config)
+static int lpss_i2c_gen_speed_config(enum i2c_speed speed,
+					const struct lpss_i2c_bus_config *bcfg,
+					struct lpss_i2c_speed_config *config)
 {
 	const int ic_clk = CONFIG_SOC_INTEL_COMMON_LPSS_I2C_CLOCK_MHZ;
 	uint16_t hcnt_min, lcnt_min;
+	int i;
 
 	/* Clock must be provided by Kconfig */
-	if (!ic_clk || !config)
+	if (!ic_clk)
 		return -1;
 
+	/* Apply board specific override for this speed if found */
+	for (i = 0; i < LPSS_I2C_SPEED_CONFIG_COUNT; i++) {
+		if (bcfg->speed_config[i].speed != speed)
+			continue;
+		memcpy(config, &bcfg->speed_config[i], sizeof(*config));
+		return 0;
+	}
+
 	if (speed >= I2C_SPEED_HIGH) {
 		/* High speed */
 		hcnt_min = MIN_HS_SCL_HIGHTIME;
@@ -478,7 +457,8 @@ int lpss_i2c_gen_speed_config(enum i2c_speed speed,
 	return 0;
 }
 
-int lpss_i2c_set_speed(unsigned bus, enum i2c_speed speed)
+static int lpss_i2c_set_speed(unsigned bus, enum i2c_speed speed,
+				const struct lpss_i2c_bus_config *bcfg)
 {
 	struct lpss_i2c_regs *regs;
 	struct lpss_i2c_speed_config config;
@@ -504,7 +484,7 @@ int lpss_i2c_set_speed(unsigned bus, enum i2c_speed speed)
 	}
 
 	/* Generate speed config based on clock */
-	if (lpss_i2c_gen_speed_config(speed, &config) < 0)
+	if (lpss_i2c_gen_speed_config(speed, bcfg, &config) < 0)
 		return -1;
 
 	/* Select this speed in the control register */
@@ -516,9 +496,37 @@ int lpss_i2c_set_speed(unsigned bus, enum i2c_speed speed)
 	return 0;
 }
 
-int lpss_i2c_init(unsigned bus, enum i2c_speed speed)
+void lpss_i2c_acpi_fill_ssdt(const struct lpss_i2c_bus_config *bcfg)
+{
+	struct lpss_i2c_speed_config sgen;
+	enum i2c_speed speeds[LPSS_I2C_SPEED_CONFIG_COUNT] = {
+		I2C_SPEED_STANDARD,
+		I2C_SPEED_FAST,
+		I2C_SPEED_FAST_PLUS,
+		I2C_SPEED_HIGH,
+	};
+	int i;
+
+	/* Report timing values for the OS driver */
+	for (i = 0; i < LPSS_I2C_SPEED_CONFIG_COUNT; i++) {
+		/* Generate speed config. */
+		if (lpss_i2c_gen_speed_config(speeds[i], bcfg, &sgen) < 0)
+			continue;
+
+		/* Generate ACPI based on selected speed config */
+		lpss_i2c_acpi_write_speed_config(&sgen);
+	}
+}
+
+int lpss_i2c_init(unsigned bus, const struct lpss_i2c_bus_config *bcfg)
 {
 	struct lpss_i2c_regs *regs;
+	enum i2c_speed speed;
+
+	if (!bcfg)
+		return -1;
+
+	speed = bcfg->speed ? : I2C_SPEED_FAST;
 
 	regs = (struct lpss_i2c_regs *)lpss_i2c_base_address(bus);
 	if (!regs) {
@@ -536,7 +544,7 @@ int lpss_i2c_init(unsigned bus, enum i2c_speed speed)
 		CONTROL_RESTART_ENABLE);
 
 	/* Set bus speed to FAST by default */
-	if (lpss_i2c_set_speed(bus, speed ? : I2C_SPEED_FAST) < 0) {
+	if (lpss_i2c_set_speed(bus, speed, bcfg) < 0) {
 		printk(BIOS_ERR, "I2C failed to set speed for bus %u\n", bus);
 		return -1;
 	}
@@ -549,7 +557,7 @@ int lpss_i2c_init(unsigned bus, enum i2c_speed speed)
 	write32(&regs->intr_mask, INTR_STAT_STOP_DET);
 
 	printk(BIOS_INFO, "LPSS I2C bus %u at 0x%p (%u KHz)\n",
-	       bus, regs, (speed ? : I2C_SPEED_FAST) / KHz);
+	       bus, regs, speed / KHz);
 
 	return 0;
 }
diff --git a/src/soc/intel/common/lpss_i2c.h b/src/soc/intel/common/lpss_i2c.h
index 3ec9213..f4a48bd 100644
--- a/src/soc/intel/common/lpss_i2c.h
+++ b/src/soc/intel/common/lpss_i2c.h
@@ -50,6 +50,15 @@ struct lpss_i2c_speed_config {
  */
 #define LPSS_I2C_SPEED_CONFIG_COUNT	4
 
+struct lpss_i2c_bus_config {
+	/* Bus should be enabled prior to ramstage with temporary base */
+	int early_init;
+	/* Bus speed in Hz, default is I2C_SPEED_FAST (400 KHz) */
+	enum i2c_speed speed;
+	/* Specific bus speed configuration */
+	struct lpss_i2c_speed_config speed_config[LPSS_I2C_SPEED_CONFIG_COUNT];
+};
+
 #define LPSS_I2C_SPEED_CONFIG(speedval,lcnt,hcnt,hold)	\
 	{						\
 		.speed = I2C_SPEED_ ## speedval,	\
@@ -67,37 +76,10 @@ struct lpss_i2c_speed_config {
 uintptr_t lpss_i2c_base_address(unsigned bus);
 
 /*
- * Generate speed configuration for requested controller and bus speed.
- *
- * This allows a SOC or board to automatically generate speed config to use
- * in firmware and provide to the OS.
- */
-int lpss_i2c_gen_speed_config(enum i2c_speed speed,
-			      struct lpss_i2c_speed_config *config);
-
-/*
- * Set raw speed configuration for given speed type.
- *
- * This allows a SOC or board to override the automatic bus speed calculation
- * and provided specific values for the driver to use.
- */
-int lpss_i2c_set_speed_config(unsigned bus,
-			      const struct lpss_i2c_speed_config *config);
-
-/*
  * Generate I2C timing information into the SSDT for the OS driver to consume,
  * optionally applying override values provided by the caller.
  */
-void lpss_i2c_acpi_fill_ssdt(const struct lpss_i2c_speed_config *override);
-
-/*
- * Set I2C bus speed for this controller.
- *
- * This allows an SOC or board to set the basic I2C bus speed.  Values for the
- * controller configuration registers will be calculated, for more specific
- * control the raw configuration can be provided to lpss_i2c_set_speed_config().
- */
-int lpss_i2c_set_speed(unsigned bus, enum i2c_speed speed);
+void lpss_i2c_acpi_fill_ssdt(const struct lpss_i2c_bus_config *bcfg);
 
 /*
  * Initialize this bus controller and set the speed.
@@ -108,6 +90,6 @@ int lpss_i2c_set_speed(unsigned bus, enum i2c_speed speed);
  * The SOC *must* define CONFIG_SOC_INTEL_COMMON_LPSS_I2C_CLOCK for the
  * bus speed calculation to be correct.
  */
-int lpss_i2c_init(unsigned bus, enum i2c_speed speed);
+int lpss_i2c_init(unsigned bus, const struct lpss_i2c_bus_config *bcfg);
 
 #endif
diff --git a/src/soc/intel/skylake/bootblock/i2c.c b/src/soc/intel/skylake/bootblock/i2c.c
index 64b1fb5..92ca861 100644
--- a/src/soc/intel/skylake/bootblock/i2c.c
+++ b/src/soc/intel/skylake/bootblock/i2c.c
@@ -46,8 +46,6 @@ static void i2c_early_init_bus(unsigned bus)
 {
 	ROMSTAGE_CONST struct soc_intel_skylake_config *config;
 	ROMSTAGE_CONST struct device *tree_dev;
-	const struct lpss_i2c_speed_config *sptr;
-	enum i2c_speed speed;
 	pci_devfn_t dev;
 	int devfn;
 	uintptr_t base;
@@ -86,17 +84,7 @@ static void i2c_early_init_bus(unsigned bus)
 	write32(reg, value);
 
 	/* Initialize the controller */
-	speed = config->i2c[bus].speed ? : I2C_SPEED_FAST;
-	lpss_i2c_init(bus, speed);
-
-	/* Apply custom speed config if it has been set by the board */
-	for (value = 0; value < LPSS_I2C_SPEED_CONFIG_COUNT; value++) {
-		sptr = &config->i2c[bus].speed_config[value];
-		if (sptr->speed == speed) {
-			lpss_i2c_set_speed_config(bus, sptr);
-			break;
-		}
-	}
+	lpss_i2c_init(bus, &config->i2c[bus]);
 }
 
 void i2c_early_init(void)
diff --git a/src/soc/intel/skylake/chip.h b/src/soc/intel/skylake/chip.h
index 81b358b..ac0f3f6 100644
--- a/src/soc/intel/skylake/chip.h
+++ b/src/soc/intel/skylake/chip.h
@@ -201,7 +201,7 @@ struct soc_intel_skylake_config {
 	/* I2C */
 	/* Bus voltage level, default is 3.3V */
 	enum skylake_i2c_voltage i2c_voltage[SKYLAKE_I2C_DEV_MAX];
-	struct skylake_i2c_config i2c[SKYLAKE_I2C_DEV_MAX];
+	struct lpss_i2c_bus_config i2c[SKYLAKE_I2C_DEV_MAX];
 
 	/* Camera */
 	u8 Cio2Enable;
diff --git a/src/soc/intel/skylake/i2c.c b/src/soc/intel/skylake/i2c.c
index d37c290..c8f0294 100644
--- a/src/soc/intel/skylake/i2c.c
+++ b/src/soc/intel/skylake/i2c.c
@@ -56,24 +56,12 @@ static int i2c_dev_to_bus(struct device *dev)
 static void i2c_dev_init(struct device *dev)
 {
 	struct soc_intel_skylake_config *config = dev->chip_info;
-	const struct lpss_i2c_speed_config *sptr;
-	enum i2c_speed speed;
-	int i, bus = i2c_dev_to_bus(dev);
+	int bus = i2c_dev_to_bus(dev);
 
 	if (!config || bus < 0)
 		return;
 
-	speed = config->i2c[bus].speed ? : I2C_SPEED_FAST;
-	lpss_i2c_init(bus, speed);
-
-	/* Apply custom speed config if it has been set by the board */
-	for (i = 0; i < LPSS_I2C_SPEED_CONFIG_COUNT; i++) {
-		sptr = &config->i2c[bus].speed_config[i];
-		if (sptr->speed == speed) {
-			lpss_i2c_set_speed_config(bus, sptr);
-			break;
-		}
-	}
+	lpss_i2c_init(bus, &config->i2c[bus]);
 }
 
 /* Generate ACPI I2C device objects */
@@ -86,7 +74,7 @@ static void i2c_fill_ssdt(struct device *dev)
 		return;
 
 	acpigen_write_scope(acpi_device_path(dev));
-	lpss_i2c_acpi_fill_ssdt(config->i2c[bus].speed_config);
+	lpss_i2c_acpi_fill_ssdt(&config->i2c[bus]);
 	acpigen_pop_len();
 }
 



More information about the coreboot-gerrit mailing list