Hello Martin Roth, Magf -,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40248
to review the following change.
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
soc/amd/picasso/i2c: do not init I2C4
I2C4 is slave device only, so do not init it as I2C host controller.
BUG=b:150916992 BRANCH=none TEST=build and test dalboz, AP mux can be accessed successfully.
Change-Id: I6a3ddecd293f550a4df5e667094c69f5c115e29c Signed-off-by: Paul Ma magf@bitland.corp-partner.google.com Reviewed-on: https://chromium-review.googlesource.com/2094855 Tested-by: Martin Roth martinroth@google.com Reviewed-by: Martin Roth martinroth@google.com --- M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 2 files changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/40248/1
diff --git a/src/soc/amd/picasso/i2c.c b/src/soc/amd/picasso/i2c.c index 2bd3116..a3e34c8 100644 --- a/src/soc/amd/picasso/i2c.c +++ b/src/soc/amd/picasso/i2c.c @@ -18,12 +18,15 @@ /* Global to provide access to chip.c */ const char *i2c_acpi_name(const struct device *dev);
+/* + * I2C4 is slave device only, do not list it here, + * otherwise this slave device will not be able to work. + */ 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, diff --git a/src/soc/amd/picasso/include/soc/iomap.h b/src/soc/amd/picasso/include/soc/iomap.h index 3f539a2..edd9ff1 100644 --- a/src/soc/amd/picasso/include/soc/iomap.h +++ b/src/soc/amd/picasso/include/soc/iomap.h @@ -26,7 +26,7 @@ #define APU_I2C4_BASE 0xfedc6000 #define I2C_BASE_ADDRESS APU_I2C0_BASE #define I2C_DEVICE_SIZE 0x00001000 -#define I2C_DEVICE_COUNT 5 +#define I2C_DEVICE_COUNT 4
#define APU_DMAC0_BASE 0xfedc7000 #define APU_DMAC1_BASE 0xfedc8000
Magf - has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40248 )
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40248 )
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40248/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40248/1//COMMIT_MSG@7 PS1, Line 7: soc/amd/picasso/i2c: do not init I2C4 … as I2C host controller
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c@2... PS1, Line 23: * otherwise this slave device will not be able to work. Please re-flow for 96 characters.
I’d prefer if you left the member in there but commented out.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40248 )
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c@2... PS1, Line 23: * otherwise this slave device will not be able to work.
Please re-flow for 96 characters. […]
did the reflow; will push it later today. I dislike keeping commented out things; also with the comment above I don't see much reason to keep a commented out APU_I2C4_BASE
Hello build bot (Jenkins), Raul Rangel, Martin Roth, Magf -,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40248
to look at the new patch set (#2).
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
soc/amd/picasso/i2c: do not init I2C4
I2C4 is slave device only, so do not init it as I2C host controller.
BUG=b:150916992 BRANCH=none TEST=build and test dalboz, AP mux can be accessed successfully.
Change-Id: I6a3ddecd293f550a4df5e667094c69f5c115e29c Signed-off-by: Paul Ma magf@bitland.corp-partner.google.com Reviewed-on: https://chromium-review.googlesource.com/2094855 Tested-by: Martin Roth martinroth@google.com Reviewed-by: Martin Roth martinroth@google.com --- M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 2 files changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/40248/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40248 )
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c@2... PS1, Line 23: * otherwise this slave device will not be able to work.
did the reflow; will push it later today. […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40248 )
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c@2... PS1, Line 23: * otherwise this slave device will not be able to work.
Done
Thanks. Understood. I would have thought, it would be easier graspable than the comment.
It’d be great to have some mention, *why* slave devices won’t work if listed here.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40248 )
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/40248/1/src/soc/amd/picasso/i2c.c@2... PS1, Line 23: * otherwise this slave device will not be able to work.
Thanks. Understood. I would have thought, it would be easier graspable than the comment. […]
If you initialize a slave as if it were a master, it won't really work I guess
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40248 )
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40248/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40248/1//COMMIT_MSG@7 PS1, Line 7: soc/amd/picasso/i2c: do not init I2C4
… as I2C host controller
or:
soc/amd/picasso/i2c: Do not init I2C4 as a master
Felix Held has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40248 )
Change subject: soc/amd/picasso/i2c: do not init I2C4 ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
Magf - (Code Review)
-
Paul Menzel (Code Review)