Hello Raul Rangel,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40247
to review the following change.
Change subject: soc/amd/common: Determine # of I2C controllers at runtime ......................................................................
soc/amd/common: Determine # of I2C controllers at runtime
Dali only has 3 I2C controllers, while Pollock has 5. Both boards use the same socket. So we determine at runtime how many I2C controllers are available.
I2C0 and I2C1 are not documented in the PPR, so I took a guess at the addressed.
BUG=b:149572620 BRANCH=none TEST=Booted dalboz firmware on trembyle. Saw only three controllers initialized. Touchscreen was still working.
Change-Id: I397b074ef9c14bf6a4f6680696582f5173a5d0d3 Signed-off-by: Martin Roth martinroth@chromium.org Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://chromium-review.googlesource.com/1897071 Reviewed-on: https://chromium-review.googlesource.com/2057468 --- A src/soc/amd/common/block/include/amdblocks/i2c.h M src/soc/amd/common/block/lpc/lpc.c M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 6 files changed, 63 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/40247/1
diff --git a/src/soc/amd/common/block/include/amdblocks/i2c.h b/src/soc/amd/common/block/include/amdblocks/i2c.h new file mode 100644 index 0000000..f1ffa71 --- /dev/null +++ b/src/soc/amd/common/block/include/amdblocks/i2c.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#ifndef __AMDBLOCKS_I2C_H__ +#define __AMDBLOCKS_I2C_H__ + +#include <device/resource.h> + +/* Some systems can only determine the number of i2c controllers at runtime. */ +void soc_update_i2c_resource(struct resource *res); + +#endif /* __AMDBLOCKS_I2C_H__ */ diff --git a/src/soc/amd/common/block/lpc/lpc.c b/src/soc/amd/common/block/lpc/lpc.c index 5346356..e84594b 100644 --- a/src/soc/amd/common/block/lpc/lpc.c +++ b/src/soc/amd/common/block/lpc/lpc.c @@ -16,6 +16,7 @@ #include <pc80/i8259.h> #include <amdblocks/acpimmio.h> #include <amdblocks/lpc.h> +#include <amdblocks/i2c.h> #include <soc/acpi.h> #include <soc/southbridge.h> #include <soc/nvs.h> @@ -24,6 +25,8 @@ /* Most systems should have already enabled the bridge */ void __weak soc_late_lpc_bridge_enable(void) { }
+void __weak soc_update_i2c_resource(struct resource *res) { } + static void lpc_init(struct device *dev) { u8 byte; @@ -118,12 +121,14 @@ res->size = 0x00001000; res->flags = IORESOURCE_MEM | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
- /* I2C devices (all 4 devices) */ + /* I2C devices */ res = new_resource(dev, 4); res->base = I2C_BASE_ADDRESS; res->size = I2C_DEVICE_SIZE * I2C_DEVICE_COUNT; res->flags = IORESOURCE_MEM | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
+ soc_update_i2c_resource(res); + compact_resources(dev);
/* Allocate ACPI NVS in CBMEM */ diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 5cef9ba..0ded6dd 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -42,6 +42,7 @@ bootblock-y += tsc_freq.c bootblock-y += gpio.c bootblock-y += smi_util.c +bootblock-y += soc_util.c
romstage-y += i2c.c romstage-y += romstage.c diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index b2ff9d8..adc9160 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -9,11 +9,10 @@ #include <commonlib/helpers.h> #include <drivers/i2c/designware/dw_i2c.h> #include <soc/i2c.h> +#include <soc/iomap.h> #include <arch/acpi_device.h> #include <FspsUpd.h>
-#define PICASSO_I2C_DEV_MAX 4 - struct soc_amd_picasso_config { /* * If sb_reset_i2c_slaves() is called, this devicetree register @@ -24,7 +23,7 @@ * register i2c_scl_reset = (GPIO_I2C0_SCL | GPIO_I2C3_SCL) */ u8 i2c_scl_reset; - struct dw_i2c_bus_config i2c[PICASSO_I2C_DEV_MAX]; + struct dw_i2c_bus_config i2c[I2C_DEVICE_COUNT]; enum { I2S_PINS_MAX_HDA = 0, /* HDA w/reset 3xSDI, SW w/Data0 */ I2S_PINS_MAX_MHDA = 1, /* HDA no reset 3xSDI, SW w/Data0-1 */ diff --git a/src/soc/amd/picasso/i2c.c b/src/soc/amd/picasso/i2c.c index 6bbc7a7..2bd3116 100644 --- a/src/soc/amd/picasso/i2c.c +++ b/src/soc/amd/picasso/i2c.c @@ -7,34 +7,41 @@ #include <delay.h> #include <drivers/i2c/designware/dw_i2c.h> #include <amdblocks/acpimmio.h> +#include <amdblocks/i2c.h> +#include <soc/i2c.h> #include <soc/iomap.h> #include <soc/pci_devs.h> +#include <soc/soc_util.h> #include <soc/southbridge.h> -#include <soc/i2c.h> #include "chip.h"
/* Global to provide access to chip.c */ const char *i2c_acpi_name(const struct device *dev);
static const uintptr_t i2c_bus_address[] = { + APU_I2C0_BASE, + APU_I2C1_BASE, APU_I2C2_BASE, APU_I2C3_BASE, APU_I2C4_BASE, /* slave device only */ };
+_Static_assert(ARRAY_SIZE(i2c_bus_address) == I2C_DEVICE_COUNT, + "ARRAY_SIZE(i2c_bus_address) must equal I2C_DEVICE_COUNT"); + uintptr_t dw_i2c_base_address(unsigned int bus) { - if (bus < APU_I2C_MIN_BUS || bus > APU_I2C_MAX_BUS) + if (bus >= ARRAY_SIZE(i2c_bus_address)) return 0;
- return i2c_bus_address[bus - APU_I2C_MIN_BUS]; + 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;
- if (bus < APU_I2C_MIN_BUS || bus > APU_I2C_MAX_BUS) + if (bus >= ARRAY_SIZE(config->i2c)) return NULL;
config = get_soc_config(); @@ -47,6 +54,10 @@ const char *i2c_acpi_name(const struct device *dev) { switch (dev->path.mmio.addr) { + case APU_I2C0_BASE: + return "I2C0"; + case APU_I2C1_BASE: + return "I2C1"; case APU_I2C2_BASE: return "I2C2"; case APU_I2C3_BASE: @@ -61,6 +72,10 @@ int dw_i2c_soc_dev_to_bus(struct device *dev) { switch (dev->path.mmio.addr) { + case APU_I2C0_BASE: + return 0; + case APU_I2C1_BASE: + return 1; case APU_I2C2_BASE: return 2; case APU_I2C3_BASE: @@ -73,6 +88,14 @@
__weak void mainboard_i2c_override(int bus, uint32_t *pad_settings) { }
+/* Pollock has access to I2C0 and I2C1. */ +static unsigned int i2c_start_index(void) +{ + if (soc_is_pollock()) + return 0; + return 2; +} + static void dw_i2c_soc_init(bool is_early_init) { size_t i; @@ -85,7 +108,7 @@ if (config == NULL) return;
- for (i = 0; i < ARRAY_SIZE(config->i2c); i++) { + for (i = i2c_start_index(); i < ARRAY_SIZE(config->i2c); i++) { const struct dw_i2c_bus_config *cfg = &config->i2c[i];
if (cfg->early_init != is_early_init) @@ -126,6 +149,15 @@ dw_i2c_soc_init(false); }
+void soc_update_i2c_resource(struct resource *res) +{ + unsigned int index = i2c_start_index(); + unsigned int count = ARRAY_SIZE(i2c_bus_address) - index; + + res->base = dw_i2c_base_address(index); + res->size = I2C_DEVICE_SIZE * count; +} + struct device_operations picasso_i2c_mmio_ops = { /* TODO(teravest): Move I2C resource info here. */ .read_resources = DEVICE_NOOP, diff --git a/src/soc/amd/picasso/include/soc/iomap.h b/src/soc/amd/picasso/include/soc/iomap.h index 0a6e1a1..3f539a2 100644 --- a/src/soc/amd/picasso/include/soc/iomap.h +++ b/src/soc/amd/picasso/include/soc/iomap.h @@ -18,15 +18,15 @@
/* Reserved 0xfecd1000-0xfedc3fff */
+/* I2C0 and I2C1 are only available on Pollock */ +#define APU_I2C0_BASE 0xfedc2000 +#define APU_I2C1_BASE 0xfedc3000 #define APU_I2C2_BASE 0xfedc4000 #define APU_I2C3_BASE 0xfedc5000 #define APU_I2C4_BASE 0xfedc6000 -#define APU_I2C_MIN_BUS 2 -#define APU_I2C_MAX_BUS 4 -#define APU_I2C_BLOCK_SIZE 0x1000 -#define I2C_BASE_ADDRESS APU_I2C2_BASE +#define I2C_BASE_ADDRESS APU_I2C0_BASE #define I2C_DEVICE_SIZE 0x00001000 -#define I2C_DEVICE_COUNT 3 +#define I2C_DEVICE_COUNT 5
#define APU_DMAC0_BASE 0xfedc7000 #define APU_DMAC1_BASE 0xfedc8000
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/common: Determine # of I2C controllers at runtime ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40247/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40247/1//COMMIT_MSG@14 PS1, Line 14: addressed addresses?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/common: Determine # of I2C controllers at runtime ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/40247/1/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/1/src/soc/amd/picasso/i2c.c@3... PS1, Line 30: "ARRAY_SIZE(i2c_bus_address) must equal I2C_DEVICE_COUNT"); Sure, but why?
https://review.coreboot.org/c/coreboot/+/40247/1/src/soc/amd/picasso/i2c.c@1... PS1, Line 154: unsigned int index = i2c_start_index(); const?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/common: Determine # of I2C controllers at runtime ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40247/2/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/2/src/soc/amd/picasso/i2c.c@2... PS2, Line 23: APU_I2C0_BASE So I think we can drop this CL. I2C0 and I2C1 are accessible via the x86, but the interrupts were never routed to the x86 domain. So the device is essentially useless.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/common: Determine # of I2C controllers at runtime ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40247/2/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/2/src/soc/amd/picasso/i2c.c@2... PS2, Line 23: APU_I2C0_BASE
So I think we can drop this CL. […]
oh, didn't know that; I do wonder though what the second patch i squashed into this was for then. I'll re-push this patch with only the first patch of the two ones I squashed then
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/common: Determine # of I2C controllers at runtime ......................................................................
Patch Set 2: Code-Review-1
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Paul Menzel, Angel Pons, Magf -,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40247
to look at the new patch set (#3).
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as slave and refactor code ......................................................................
soc/amd/picasso/i2c: don't initialize I2C4 as slave and refactor code
I2C0&1 don't are either not available or not functional. Add place holders instead, so that the array index matches the I2C controller number. I2C4 is slave device only, so do not initialize it as I2C host controller. Also some slight refactoring.
BUG=b:153152871 BUG=b:153675916
Change-Id: I397b074ef9c14bf6a4f6680696582f5173a5d0d3 Signed-off-by: Martin Roth martinroth@chromium.org Signed-off-by: Paul Ma magf@bitland.corp-partner.google.com Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://chromium-review.googlesource.com/1897071 Reviewed-on: https://chromium-review.googlesource.com/2057468 Reviewed-on: https://chromium-review.googlesource.com/2094855 Reviewed-on: https://chromium-review.googlesource.com/2149870 --- M src/soc/amd/common/block/lpc/lpc.c M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 4 files changed, 32 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/40247/3
Magf - has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as slave and refactor code ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40247/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40247/3//COMMIT_MSG@7 PS3, Line 7: soc/amd/picasso/i2c: don't initialize I2C4 as slave and refactor code don't initialize I2C4 as master and refactor code
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Paul Menzel, Angel Pons, Magf -,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40247
to look at the new patch set (#4).
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code
I2C0&1 don't are either not available or not functional. Add place holders instead, so that the array index matches the I2C controller number. I2C4 is slave device only, so do not initialize it as I2C host controller. Also some slight refactoring.
BUG=b:153152871 BUG=b:153675916
Change-Id: I397b074ef9c14bf6a4f6680696582f5173a5d0d3 Signed-off-by: Martin Roth martinroth@chromium.org Signed-off-by: Paul Ma magf@bitland.corp-partner.google.com Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://chromium-review.googlesource.com/1897071 Reviewed-on: https://chromium-review.googlesource.com/2057468 Reviewed-on: https://chromium-review.googlesource.com/2094855 Reviewed-on: https://chromium-review.googlesource.com/2149870 --- M src/soc/amd/common/block/lpc/lpc.c M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 4 files changed, 32 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/40247/4
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40247/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40247/3//COMMIT_MSG@7 PS3, Line 7: soc/amd/picasso/i2c: don't initialize I2C4 as slave and refactor code
don't initialize I2C4 as master and refactor code
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40247/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40247/1//COMMIT_MSG@14 PS1, Line 14: addressed
addresses?
Done
https://review.coreboot.org/c/coreboot/+/40247/1/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/1/src/soc/amd/picasso/i2c.c@3... PS1, Line 30: "ARRAY_SIZE(i2c_bus_address) must equal I2C_DEVICE_COUNT");
Sure, but why?
Done
https://review.coreboot.org/c/coreboot/+/40247/2/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/2/src/soc/amd/picasso/i2c.c@2... PS2, Line 23: APU_I2C0_BASE
oh, didn't know that; I do wonder though what the second patch i squashed into this was for then. […]
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 4:
(1 comment)
marked the comments from the old change set as solved. should maybe have used a new change ID
https://review.coreboot.org/c/coreboot/+/40247/1/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/1/src/soc/amd/picasso/i2c.c@1... PS1, Line 154: unsigned int index = i2c_start_index();
const?
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 4: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/40247/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40247/4//COMMIT_MSG@9 PS4, Line 9: don't Remove
https://review.coreboot.org/c/coreboot/+/40247/4//COMMIT_MSG@12 PS4, Line 12: Also some Also *do* some
https://review.coreboot.org/c/coreboot/+/40247/4/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/4/src/soc/amd/picasso/i2c.c@2... PS4, Line 24: [] If the array size was "I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT", would the _Static_assert still be needed? It would definitely fail to build if there are excess elements in the array initializer, but not sure what would happen if there are less.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
I'll +2 once https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... lands.
https://review.coreboot.org/c/coreboot/+/40247/4/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/4/src/soc/amd/picasso/i2c.c@2... PS4, Line 24: []
If the array size was "I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT", would the _Static_assert still b […]
I left it this way so that if the #defines are changed or the array is changed it will fail. Explicitly setting the array catches the case where the number of elements listed is less.
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Paul Menzel, Angel Pons, Magf -,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40247
to look at the new patch set (#5).
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code
I2C0&1 are either not available or not functional. Add place holders instead, so that the array index matches the I2C controller number. I2C4 is slave device only, so do not initialize it as I2C host controller. Also do some slight refactoring.
BUG=b:153152871 BUG=b:153675916
Change-Id: I397b074ef9c14bf6a4f6680696582f5173a5d0d3 Signed-off-by: Martin Roth martinroth@chromium.org Signed-off-by: Paul Ma magf@bitland.corp-partner.google.com Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://chromium-review.googlesource.com/1897071 Reviewed-on: https://chromium-review.googlesource.com/2057468 Reviewed-on: https://chromium-review.googlesource.com/2094855 Reviewed-on: https://chromium-review.googlesource.com/2149870 --- M src/soc/amd/common/block/lpc/lpc.c M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 4 files changed, 29 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/40247/5
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40247/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40247/4//COMMIT_MSG@9 PS4, Line 9: don't
Remove
Done
https://review.coreboot.org/c/coreboot/+/40247/4//COMMIT_MSG@12 PS4, Line 12: Also some
Also *do* some
Done
https://review.coreboot.org/c/coreboot/+/40247/4/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/4/src/soc/amd/picasso/i2c.c@2... PS4, Line 24: []
If the array size was "I2C_MASTER_DEV_COUNT + I2C_SLAVE_DEV_COUNT", would the _Static_assert still b […]
if there are more, it's a compilation error and if there are less, the rest will get zero-initialized. Also the code calling dw_i2c_base_address checks if it's zero and bails out with an error message if that's the case
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40247/4/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/4/src/soc/amd/picasso/i2c.c@2... PS4, Line 24: []
if there are more, it's a compilation error and if there are less, the rest will get zero-initialize […]
Ack
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 5: Code-Review-1
there's still some work going on in downstream, so this patch might not be final yet
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Paul Menzel, Angel Pons, Magf -,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40247
to look at the new patch set (#6).
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code
I2C0&1 are either not available or not functional. Add place holders instead, so that the array index matches the I2C controller number. I2C4 is slave device only, so do not initialize it as I2C host controller. Also do some slight refactoring.
BUG=b:153152871 BUG=b:153675916
Change-Id: I397b074ef9c14bf6a4f6680696582f5173a5d0d3 Signed-off-by: Martin Roth martinroth@chromium.org Signed-off-by: Paul Ma magf@bitland.corp-partner.google.com Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://chromium-review.googlesource.com/1897071 Reviewed-on: https://chromium-review.googlesource.com/2057468 Reviewed-on: https://chromium-review.googlesource.com/2094855 Reviewed-on: https://chromium-review.googlesource.com/2149870 --- M src/soc/amd/common/block/lpc/lpc.c M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 4 files changed, 35 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/40247/6
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
Much better now.
https://review.coreboot.org/c/coreboot/+/40247/6/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/40247/6/src/soc/amd/picasso/include... PS6, Line 27: we Fits on line above.
Raul Rangel has uploaded a new patch set (#7) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code
I2C0&1 are either not available or not functional. Add place holders instead, so that the array index matches the I2C controller number. I2C4 is slave device only, so do not initialize it as I2C host controller. Also do some slight refactoring.
BUG=b:153152871 BUG=b:153675916
Change-Id: I397b074ef9c14bf6a4f6680696582f5173a5d0d3 Signed-off-by: Martin Roth martinroth@chromium.org Signed-off-by: Paul Ma magf@bitland.corp-partner.google.com Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://chromium-review.googlesource.com/1897071 Reviewed-on: https://chromium-review.googlesource.com/2057468 Reviewed-on: https://chromium-review.googlesource.com/2094855 Reviewed-on: https://chromium-review.googlesource.com/2149870 --- M src/soc/amd/common/block/lpc/lpc.c M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 4 files changed, 35 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/40247/7
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
Fixed a nit for Felix since he is OOO.
https://review.coreboot.org/c/coreboot/+/40247/6/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/40247/6/src/soc/amd/picasso/include... PS6, Line 27: we
Fits on line above.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 7: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 7: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 7: Code-Review+2
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code
I2C0&1 are either not available or not functional. Add place holders instead, so that the array index matches the I2C controller number. I2C4 is slave device only, so do not initialize it as I2C host controller. Also do some slight refactoring.
BUG=b:153152871 BUG=b:153675916
Change-Id: I397b074ef9c14bf6a4f6680696582f5173a5d0d3 Signed-off-by: Martin Roth martinroth@chromium.org Signed-off-by: Paul Ma magf@bitland.corp-partner.google.com Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-on: https://chromium-review.googlesource.com/1897071 Reviewed-on: https://chromium-review.googlesource.com/2057468 Reviewed-on: https://chromium-review.googlesource.com/2094855 Reviewed-on: https://chromium-review.googlesource.com/2149870 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40247 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/block/lpc/lpc.c M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 4 files changed, 35 insertions(+), 17 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Raul Rangel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/soc/amd/common/block/lpc/lpc.c b/src/soc/amd/common/block/lpc/lpc.c index 5346356..3df5ad0 100644 --- a/src/soc/amd/common/block/lpc/lpc.c +++ b/src/soc/amd/common/block/lpc/lpc.c @@ -118,7 +118,7 @@ res->size = 0x00001000; res->flags = IORESOURCE_MEM | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
- /* I2C devices (all 4 devices) */ + /* I2C devices */ res = new_resource(dev, 4); res->base = I2C_BASE_ADDRESS; res->size = I2C_DEVICE_SIZE * I2C_DEVICE_COUNT; diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index 53c0329..9c756ed 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -9,10 +9,9 @@ #include <commonlib/helpers.h> #include <drivers/i2c/designware/dw_i2c.h> #include <soc/i2c.h> +#include <soc/iomap.h> #include <arch/acpi_device.h>
-#define PICASSO_I2C_DEV_MAX 4 - struct soc_amd_picasso_config { /* * If sb_reset_i2c_slaves() is called, this devicetree register @@ -23,7 +22,7 @@ * register i2c_scl_reset = (GPIO_I2C0_SCL | GPIO_I2C3_SCL) */ u8 i2c_scl_reset; - struct dw_i2c_bus_config i2c[PICASSO_I2C_DEV_MAX]; + struct dw_i2c_bus_config i2c[I2C_MASTER_DEV_COUNT]; enum { I2S_PINS_MAX_HDA = 0, /* HDA w/reset 3xSDI, SW w/Data0 */ I2S_PINS_MAX_MHDA = 1, /* HDA no reset 3xSDI, SW w/Data0-1 */ diff --git a/src/soc/amd/picasso/i2c.c b/src/soc/amd/picasso/i2c.c index 22c6216..8ba05aa 100644 --- a/src/soc/amd/picasso/i2c.c +++ b/src/soc/amd/picasso/i2c.c @@ -8,34 +8,39 @@ #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/southbridge.h> -#include <soc/i2c.h> #include "chip.h"
/* Global to provide access to chip.c */ const char *i2c_acpi_name(const struct device *dev);
-static const uintptr_t i2c_bus_address[] = { +/* + * 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, /* slave device only */ + APU_I2C4_BASE, /* Can only be used in slave mode */ };
uintptr_t dw_i2c_base_address(unsigned int bus) { - if (bus < APU_I2C_MIN_BUS || bus > APU_I2C_MAX_BUS) + if (bus >= ARRAY_SIZE(i2c_bus_address)) return 0;
- return i2c_bus_address[bus - APU_I2C_MIN_BUS]; + 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;
- if (bus < APU_I2C_MIN_BUS || bus > APU_I2C_MAX_BUS) + if (bus >= ARRAY_SIZE(config->i2c)) return NULL;
/* config is not NULL; if it was, config_of_soc calls die() internally */ @@ -83,7 +88,7 @@ /* config is not NULL; if it was, config_of_soc calls die() internally */ config = config_of_soc();
- for (i = 0; i < ARRAY_SIZE(config->i2c); i++) { + for (i = I2C_MASTER_START_INDEX; i < ARRAY_SIZE(config->i2c); i++) { const struct dw_i2c_bus_config *cfg = &config->i2c[i];
if (cfg->early_init != is_early_init) diff --git a/src/soc/amd/picasso/include/soc/iomap.h b/src/soc/amd/picasso/include/soc/iomap.h index 1ff3440..5e1e6e2 100644 --- a/src/soc/amd/picasso/include/soc/iomap.h +++ b/src/soc/amd/picasso/include/soc/iomap.h @@ -18,15 +18,29 @@
/* Reserved 0xfecd1000-0xfedc3fff */
+/* + * Picasso/Dali have I2C0 and I2C1 wired to the Sensor Fusion Hub (SFH/MP2). + * The controllers are not directly accessible via the x86. + * + * On Pollock, I2C0 and I2C1 are routed to the x86 domain, but unfortunately the + * interrupts weren't. This effectively makes the I2C controllers useless, so we + * pretend they don't exist. + * + * We want the device tree numbering to match the I2C numbers, so we allocate + * I2C0 and I2C1 even though they are not functional. + */ +#define I2C_MASTER_DEV_COUNT 4 +#define I2C_MASTER_START_INDEX 2 +#define I2C_SLAVE_DEV_COUNT 1 + #define APU_I2C2_BASE 0xfedc4000 #define APU_I2C3_BASE 0xfedc5000 #define APU_I2C4_BASE 0xfedc6000 -#define APU_I2C_MIN_BUS 2 -#define APU_I2C_MAX_BUS 4 -#define APU_I2C_BLOCK_SIZE 0x1000 -#define I2C_BASE_ADDRESS APU_I2C2_BASE -#define I2C_DEVICE_SIZE 0x00001000 -#define I2C_DEVICE_COUNT 3 + +/* I2C parameters for lpc_read_resources */ +#define I2C_BASE_ADDRESS APU_I2C2_BASE +#define I2C_DEVICE_SIZE 0x00001000 +#define I2C_DEVICE_COUNT 3
#define APU_DMAC0_BASE 0xfedc7000 #define APU_DMAC1_BASE 0xfedc8000
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 8:
Automatic boot test returned (PASS/FAIL/TOTAL): 5/0/5 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2628 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2627 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2626 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2625 Non-emulation targets: "HP Z220 SFF Workstation" using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/2629
Please note: This test is under development and might not be accurate at all!
Edward Hill has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40247/8/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/8/src/soc/amd/picasso/i2c.c@9... PS8, Line 91: ARRAY_SIZE(config->i2c) Is this wrongly including APU_I2C4_BASE?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40247 )
Change subject: soc/amd/picasso/i2c: don't initialize I2C4 as master and refactor code ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40247/8/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40247/8/src/soc/amd/picasso/i2c.c@9... PS8, Line 91: ARRAY_SIZE(config->i2c)
Is this wrongly including APU_I2C4_BASE?
I don't think so.
I2C_MASTER_DEV_COUNT = 2. ARRAY_SIZE(config->i2c) = 4. It's declared as: struct dw_i2c_bus_config i2c[I2C_MASTER_DEV_COUNT];
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Edward Hill (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Magf - (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)