Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
soc/amd/picasso: Split I2c implementation for psp_verstage
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/Makefile.inc A src/soc/amd/picasso/i2c-psp.c A src/soc/amd/picasso/i2c-x86.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 5 files changed, 86 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/1
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index ce0a7a8..250b9d1 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -13,6 +13,7 @@ bootblock-y += bootblock/pre_c.S bootblock-y += bootblock/bootblock.c bootblock-y += southbridge.c +bootblock-y += i2c-x86.c bootblock-y += i2c.c bootblock-$(CONFIG_PICASSO_UART) += uart.c bootblock-y += tsc_freq.c @@ -21,6 +22,7 @@ bootblock-y += config.c
romstage-y += i2c.c +romstage-y += i2c-x86.c romstage-y += romstage.c romstage-y += gpio.c romstage-y += pmutil.c @@ -37,10 +39,16 @@ verstage-y += i2c.c verstage-y += pmutil.c verstage-y += config.c +ifeq ($(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK),y) +verstage-y += i2c-psp.c +else +verstage-y += i2c-x86.c +endif verstage-$(CONFIG_PICASSO_UART) += uart.c verstage-y += tsc_freq.c
ramstage-y += i2c.c +ramstage-y += i2c-x86.c ramstage-y += chip.c ramstage-y += cpu.c ramstage-y += data_fabric_util.c diff --git a/src/soc/amd/picasso/i2c-psp.c b/src/soc/amd/picasso/i2c-psp.c new file mode 100644 index 0000000..c4840cd --- /dev/null +++ b/src/soc/amd/picasso/i2c-psp.c @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#include <commonlib/helpers.h> +#include <console/console.h> +#include <drivers/i2c/designware/dw_i2c.h> +#include <soc/i2c.h> +#include <soc/iomap.h> +#include <stdint.h> + +static uintptr_t i2c_bus_address[I2C_MASTER_DEV_COUNT]; + +uintptr_t dw_i2c_base_address(unsigned int bus) +{ + if (bus >= ARRAY_SIZE(i2c_bus_address)) + return 0; + + return i2c_bus_address[bus]; +} +void i2c_set_bar(unsigned int bus, uintptr_t bar) +{ + if (bus >= ARRAY_SIZE(i2c_bus_address)) { + printk(BIOS_ERR, "Error: i2c index out of bounds: %u.", bus); + return; + } + + i2c_bus_address[bus] = bar; +} diff --git a/src/soc/amd/picasso/i2c-x86.c b/src/soc/amd/picasso/i2c-x86.c new file mode 100644 index 0000000..444a430 --- /dev/null +++ b/src/soc/amd/picasso/i2c-x86.c @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#include <stdint.h> +#include <soc/iomap.h> +#include <commonlib/helpers.h> +#include <drivers/i2c/designware/dw_i2c.h> + +/* + * We don't have addresses for I2C0-1. + */ +static const uintptr_t i2c_bus_address[] = { + 0, + 0, + APU_I2C2_BASE, + APU_I2C3_BASE, + APU_I2C4_BASE, /* Can only be used in slave mode */ +}; + +_Static_assert( + ARRAY_SIZE(i2c_bus_address) == I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT, + "ARRAY_SIZE(i2c_bus_address) must equal I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT"); + +uintptr_t dw_i2c_base_address(unsigned int bus) +{ + if (bus >= ARRAY_SIZE(i2c_bus_address)) + return 0; + + return i2c_bus_address[bus]; +} diff --git a/src/soc/amd/picasso/i2c.c b/src/soc/amd/picasso/i2c.c index 4c8c669..4818455 100644 --- a/src/soc/amd/picasso/i2c.c +++ b/src/soc/amd/picasso/i2c.c @@ -1,40 +1,18 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <device/mmio.h> -#include <acpi/acpi.h> #include <console/console.h> #include <delay.h> -#include <device/device.h> #include <drivers/i2c/designware/dw_i2c.h> #include <amdblocks/acpimmio.h> #include <soc/i2c.h> -#include <soc/iomap.h> -#include <soc/pci_devs.h> +#include <soc/soc_util.h> #include <soc/southbridge.h> #include "chip.h"
/* Global to provide access to chip.c */ const char *i2c_acpi_name(const struct device *dev);
-/* - * We don't have addresses for I2C0-1. - */ -static const uintptr_t i2c_bus_address[I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT] = { - 0, - 0, - APU_I2C2_BASE, - APU_I2C3_BASE, - APU_I2C4_BASE, /* Can only be used in slave mode */ -}; - -uintptr_t dw_i2c_base_address(unsigned int bus) -{ - if (bus >= ARRAY_SIZE(i2c_bus_address)) - return 0; - - return i2c_bus_address[bus]; -} - const struct dw_i2c_bus_config *dw_i2c_get_soc_cfg(unsigned int bus) { const struct soc_amd_picasso_config *config; @@ -159,7 +137,7 @@ { uint32_t *gpio_ptr;
- gpio_ptr = (uint32_t *)gpio_get_address(gpio); + gpio_ptr = gpio_get_address(gpio); save_table->mux_value = iomux_read8(gpio); save_table->control_value = read32(gpio_ptr); } @@ -169,7 +147,7 @@ { uint32_t *gpio_ptr;
- gpio_ptr = (uint32_t *)gpio_get_address(gpio); + gpio_ptr = gpio_get_address(gpio); iomux_write8(gpio, save_table->mux_value); iomux_read8(gpio); write32(gpio_ptr, save_table->control_value); @@ -180,13 +158,17 @@ void sb_reset_i2c_slaves(void) { const struct soc_amd_picasso_config *cfg; - const struct device *dev = pcidev_path_on_root(GNB_DEVFN); struct soc_amd_i2c_save save_table[saved_pins_count]; uint8_t i, j, control; + /* I2C0-1 is not accessible from the x86. */ + uint32_t *i2c_gpio_ptr[] = { + NULL, + NULL, + gpio_get_address(I2C2_SCL_PIN), + gpio_get_address(I2C3_SCL_PIN), + };
- if (!dev || !dev->chip_info) - return; - cfg = dev->chip_info; + cfg = config_of_soc(); control = cfg->i2c_scl_reset & GPIO_I2C_MASK; if (control == 0) return; @@ -202,19 +184,19 @@ */ for (j = 0; j < 9; j++) { if (control & GPIO_I2C2_SCL) - write32((uint32_t *)GPIO_I2C2_ADDRESS, GPIO_SCL_LOW); + write32(i2c_gpio_ptr[2], GPIO_SCL_LOW); if (control & GPIO_I2C3_SCL) - write32((uint32_t *)GPIO_I2C3_ADDRESS, GPIO_SCL_LOW); + write32(i2c_gpio_ptr[3], GPIO_SCL_LOW);
- read32((uint32_t *)GPIO_I2C3_ADDRESS); /* Flush posted write */ + read32(i2c_gpio_ptr[3]); /* Flush posted write */ udelay(4); /* 4usec gets 85KHz for 1 pin, 70KHz for 4 pins */
if (control & GPIO_I2C2_SCL) - write32((uint32_t *)GPIO_I2C2_ADDRESS, GPIO_SCL_HIGH); + write32(i2c_gpio_ptr[2], GPIO_SCL_HIGH); if (control & GPIO_I2C3_SCL) - write32((uint32_t *)GPIO_I2C3_ADDRESS, GPIO_SCL_HIGH); + write32(i2c_gpio_ptr[3], GPIO_SCL_HIGH);
- read32((uint32_t *)GPIO_I2C3_ADDRESS); /* Flush posted write */ + read32(i2c_gpio_ptr[3]); /* Flush posted write */ udelay(4); }
diff --git a/src/soc/amd/picasso/include/soc/i2c.h b/src/soc/amd/picasso/include/soc/i2c.h index 58c27a4..f468d74 100644 --- a/src/soc/amd/picasso/include/soc/i2c.h +++ b/src/soc/amd/picasso/include/soc/i2c.h @@ -27,4 +27,7 @@
void sb_reset_i2c_slaves(void);
+/* Sets the base address for the specific I2C bus. */ +void i2c_set_bar(unsigned int bus, uintptr_t bar); + #endif /* __PICASSO_I2C_H__ */
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 2: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c@a... PS2, Line 7: #include <device/device.h> don't remove. config_of_soc() is used in new code added
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c@9 PS2, Line 9: #include <soc/soc_util.h> remove. isn't used and can only be used in ramstage (ok, after fsp-m wrote the hobs to be precise) anyway
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c@1... PS2, Line 140: gpio_ptr = gpio_get_address(gpio); gpio_get_address() returns one of ACPIMMIO_GPIOx_BASE and is only valid for x86.
Remaining changes in this file don't immediately look like psp-verstage related splitting.
Hello build bot (Jenkins), Patrick Georgi, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#3).
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
soc/amd/picasso: Split I2c implementation for psp_verstage
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/Makefile.inc A src/soc/amd/picasso/i2c-psp.c A src/soc/amd/picasso/i2c-x86.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 5 files changed, 69 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/3
Hello build bot (Jenkins), Patrick Georgi, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#4).
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
soc/amd/picasso: Split I2c implementation for psp_verstage
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/Makefile.inc A src/soc/amd/picasso/i2c-psp.c A src/soc/amd/picasso/i2c-x86.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 5 files changed, 68 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/4
Hello build bot (Jenkins), Patrick Georgi, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#5).
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
soc/amd/picasso: Split I2c implementation for psp_verstage
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/Makefile.inc A src/soc/amd/picasso/i2c-psp.c A src/soc/amd/picasso/i2c-x86.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 5 files changed, 68 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/5
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c@a... PS2, Line 7: #include <device/device.h>
don't remove. […]
Done
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c@9 PS2, Line 9: #include <soc/soc_util.h>
remove. […]
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... File src/soc/amd/picasso/i2c-x86.c:
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... PS5, Line 11: i2c_bus_address[] i2c_bus_address[I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT]
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... PS5, Line 19: _Static_assert( : ARRAY_SIZE(i2c_bus_address) == I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT, : "ARRAY_SIZE(i2c_bus_address) must equal I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT"); remove
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42064/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42064/5//COMMIT_MSG@7 PS5, Line 7: soc/amd/picasso: Split I2c implementation for psp_verstage i2c or I2C. First is more readable to my eyes.
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-psp... File src/soc/amd/picasso/i2c-psp.c:
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-psp... PS5, Line 7: #include <soc/iomap.h> I know I2C_MASTER_DEV_COUNT is there, but maybe it would be better moved to <soc/i2c.h>. This <soc/iomap.h> has lots of _BASE defines that are not valid in PSP context.
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c.c@1... PS5, Line 169: cfg = config_of_soc(); Unrelated. This currently brings in the entire static devicetree / static.c file into PSP verstage, possibly doubling the binary size. There are some initiatives and interest to avoid this, but nothing immediately available.
As long as you don't lock anything early, it does not matter that much if read-only (?) verstage carries different configuration vs later stages.
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 6: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 8: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 9: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42064/9/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42064/9/src/soc/amd/picasso/Makefil... PS9, Line 48: verstage-y Could make this verstage_arm-y, and verstage_x86-y
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42064/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42064/5//COMMIT_MSG@7 PS5, Line 7: soc/amd/picasso: Split I2c implementation for psp_verstage
i2c or I2C. First is more readable to my eyes.
Done
https://review.coreboot.org/c/coreboot/+/42064/9/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42064/9/src/soc/amd/picasso/Makefil... PS9, Line 48: verstage-y
Could make this verstage_arm-y, and verstage_x86-y
I'm going to look at recombining these into a single file in a follow-on patch. If I can't, i'll update this to be cleaner then.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#10).
Change subject: soc/amd/picasso: Split i2c implementation for psp_verstage ......................................................................
soc/amd/picasso: Split i2c implementation for psp_verstage
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/Makefile.inc A src/soc/amd/picasso/i2c-psp.c A src/soc/amd/picasso/i2c-x86.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 5 files changed, 67 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/10
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#11).
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
soc/amd/picasso: Split I2c implementation for psp_verstage
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/Makefile.inc A src/soc/amd/picasso/i2c-psp.c A src/soc/amd/picasso/i2c-x86.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 5 files changed, 67 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/11
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#12).
Change subject: soc/amd/picasso: Split i2c implementation for psp_verstage ......................................................................
soc/amd/picasso: Split i2c implementation for psp_verstage
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/Makefile.inc A src/soc/amd/picasso/i2c-psp.c A src/soc/amd/picasso/i2c-x86.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 5 files changed, 67 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/12
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split i2c implementation for psp_verstage ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... File src/soc/amd/picasso/i2c-x86.c:
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... PS5, Line 11: i2c_bus_address[]
i2c_bus_address[I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT]
Fixing in follow-on patch.
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... PS5, Line 19: _Static_assert( : ARRAY_SIZE(i2c_bus_address) == I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT, : "ARRAY_SIZE(i2c_bus_address) must equal I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT");
remove
fixing in follow-on patch
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/2/src/soc/amd/picasso/i2c.c@1... PS2, Line 140: gpio_ptr = gpio_get_address(gpio);
gpio_get_address() returns one of ACPIMMIO_GPIOx_BASE and is only valid for x86. […]
Done
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c.c@1... PS5, Line 169: cfg = config_of_soc();
Unrelated. This currently brings in the entire static devicetree / static. […]
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split i2c implementation for psp_verstage ......................................................................
Patch Set 12:
(4 comments)
I prefer an approach of guarding the sections with ENV_X86 instead of forking the file and that you would rebase work on tree with CB:42523 applied. In my opinion you need to be able to build psp-verstage without the x86-centric #defines of various base addresses.
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c@... PS12, Line 4: #include <acpi/acpi.h> unrelated
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c@... PS12, Line 11: #include <soc/iomap.h> needed
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c@... PS12, Line 12: #include <soc/pci_devs.h> unrelated
Could have been done earlier with config_of_soc() change I believe.
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c@... PS12, Line 51: case APU_I2C2_BASE: from <soc/iomap.h>
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#13).
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
soc/amd/picasso: Allow modification of i2c base addresses in PSP
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 2 files changed, 25 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/13
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#14).
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
soc/amd/picasso: Allow modification of i2c base addresses in PSP
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 2 files changed, 24 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/14
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 14:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... File src/soc/amd/picasso/i2c-x86.c:
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... PS5, Line 19: _Static_assert( : ARRAY_SIZE(i2c_bus_address) == I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT, : "ARRAY_SIZE(i2c_bus_address) must equal I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT");
fixing in follow-on patch
Done
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c@... PS12, Line 4: #include <acpi/acpi.h>
unrelated
Done
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c@... PS12, Line 11: #include <soc/iomap.h>
needed
Done
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c@... PS12, Line 12: #include <soc/pci_devs.h>
unrelated […]
Done
https://review.coreboot.org/c/coreboot/+/42064/12/src/soc/amd/picasso/i2c.c@... PS12, Line 51: case APU_I2C2_BASE:
from <soc/iomap. […]
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... File src/soc/amd/picasso/i2c-x86.c:
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-x86... PS5, Line 19: _Static_assert( : ARRAY_SIZE(i2c_bus_address) == I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT, : "ARRAY_SIZE(i2c_bus_address) must equal I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT");
fixing in follow-on patch
Ack
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c@... PS14, Line 28: APU_I2C2_BASE For ARM, this will have x86 addresses by default. I think Kyösti wanted to make sure there were no x86 addresses in the PSP.
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c@... PS14, Line 71: APU_I2C2_BASE Think that also includes these.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c@... PS14, Line 28: APU_I2C2_BASE
For ARM, this will have x86 addresses by default. […]
While that's obviously doable, I don't think it's an actual worry, and makes the code more complex. If we need to remove the x86 addresses, I'd prefer to to that in a follow-on patch.
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c@... PS14, Line 71: APU_I2C2_BASE
Think that also includes these.
This function isn't used in the arm code, so will be remove by the linker. Because of that, I don't think this is a concern at all. Obviously this can be rewritten to something like this, but I don't see the need.
if (dev->path.mmio.addr == dw_i2c_base_address[2]) return "I2C2"; else if (dev->path.mmio.addr == dw_i2c_base_address[3]) return "I2C3"; else if (dev->path.mmio.addr == dw_i2c_base_address[4]) return "I2C4"; else return NULL;
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#15).
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
soc/amd/picasso: Allow modification of i2c base addresses in PSP
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 2 files changed, 26 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/15
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c@... PS14, Line 28: APU_I2C2_BASE
While that's obviously doable, I don't think it's an actual worry, and makes the code more complex. […]
Done
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c@... PS14, Line 71: APU_I2C2_BASE
This function isn't used in the arm code, so will be remove by the linker. […]
Done
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c@... PS14, Line 85: APU_I2C2_BASE Figured I better get these too.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/14/src/soc/amd/picasso/i2c.c@... PS14, Line 85: APU_I2C2_BASE
Figured I better get these too.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 15: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42064/15/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/15/src/soc/amd/picasso/i2c.c@... PS15, Line 25: = {0} Not required
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#16).
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
soc/amd/picasso: Allow modification of i2c base addresses in PSP
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 2 files changed, 26 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/16
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42064/15/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/15/src/soc/amd/picasso/i2c.c@... PS15, Line 25: = {0}
Not required
Yeah I know, it's a style preference, but I'll remove it.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 16: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42064/16/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/16/src/soc/amd/picasso/i2c.c@... PS16, Line 63: uint32_t Sorry, I didn't catch this earlier...
uintptr_t
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42064
to look at the new patch set (#17).
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
soc/amd/picasso: Allow modification of i2c base addresses in PSP
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c --- M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 2 files changed, 26 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/42064/17
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42064/16/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/16/src/soc/amd/picasso/i2c.c@... PS16, Line 63: uint32_t
Sorry, I didn't catch this earlier... […]
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
Patch Set 17: Code-Review+2
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Allow modification of i2c base addresses in PSP ......................................................................
soc/amd/picasso: Allow modification of i2c base addresses in PSP
BUG=b:158124527 TEST=Build & boot psp_verstage on trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I45380e0c61e1bb7a94a96630e5867b7ffca0909c Reviewed-on: https://review.coreboot.org/c/coreboot/+/42064 Reviewed-by: Raul Rangel rrangel@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h 2 files changed, 26 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/i2c.c b/src/soc/amd/picasso/i2c.c index 881278f..1eb0720 100644 --- a/src/soc/amd/picasso/i2c.c +++ b/src/soc/amd/picasso/i2c.c @@ -13,9 +13,7 @@ #include <soc/southbridge.h> #include "chip.h"
-/* - * We don't have addresses for I2C0-1. - */ +#if ENV_X86 static const uintptr_t i2c_bus_address[I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT] = { 0, 0, @@ -23,6 +21,9 @@ APU_I2C3_BASE, APU_I2C4_BASE, /* Can only be used in slave mode */ }; +#else +static uintptr_t i2c_bus_address[I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT]; +#endif
uintptr_t dw_i2c_base_address(unsigned int bus) { @@ -32,6 +33,18 @@ return i2c_bus_address[bus]; }
+#if !ENV_X86 +void i2c_set_bar(unsigned int bus, uintptr_t bar) +{ + if (bus >= ARRAY_SIZE(i2c_bus_address)) { + printk(BIOS_ERR, "Error: i2c index out of bounds: %u.", bus); + return; + } + + i2c_bus_address[bus] = bar; +} +#endif + const struct dw_i2c_bus_config *dw_i2c_get_soc_cfg(unsigned int bus) { const struct soc_amd_picasso_config *config; @@ -47,28 +60,23 @@
static const char *i2c_acpi_name(const struct device *dev) { - switch (dev->path.mmio.addr) { - case APU_I2C2_BASE: + if ((uintptr_t)dev->path.mmio.addr == i2c_bus_address[2]) return "I2C2"; - case APU_I2C3_BASE: + else if ((uintptr_t)dev->path.mmio.addr == i2c_bus_address[3]) return "I2C3"; - case APU_I2C4_BASE: + else if ((uintptr_t)dev->path.mmio.addr == i2c_bus_address[4]) return "I2C4"; - default: - return NULL; - } + return NULL; }
int dw_i2c_soc_dev_to_bus(const struct device *dev) { - switch (dev->path.mmio.addr) { - case APU_I2C2_BASE: + if ((uintptr_t)dev->path.mmio.addr == i2c_bus_address[2]) return 2; - case APU_I2C3_BASE: + else if ((uintptr_t)dev->path.mmio.addr == i2c_bus_address[3]) return 3; - case APU_I2C4_BASE: + else if ((uintptr_t)dev->path.mmio.addr == i2c_bus_address[4]) return 4; - } return -1; }
diff --git a/src/soc/amd/picasso/include/soc/i2c.h b/src/soc/amd/picasso/include/soc/i2c.h index 34c19aa..20084f0 100644 --- a/src/soc/amd/picasso/include/soc/i2c.h +++ b/src/soc/amd/picasso/include/soc/i2c.h @@ -22,4 +22,7 @@
void sb_reset_i2c_slaves(void);
+/* Sets the base address for the specific I2C bus. */ +void i2c_set_bar(unsigned int bus, uintptr_t bar); + #endif /* __PICASSO_I2C_H__ */
-
Aaron Durbin (Code Review) -
Eric Peers (Code Review)
-
Felix Held (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Raul Rangel (Code Review)