Hello Iru Cai,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/44975
to review the following change.
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
ec: Add support for MEC5055 for Dell laptops
This EC support code is required to boot various Dell Latitude laptops, otherwise the laptop will shut down in a minute after powered on.
Tested on Dell Latitude E7240.
Change-Id: Iee68ea52dcf0242315868a46b5e4768303e30dce Signed-off-by: Iru Cai mytbk920423@gmail.com --- A src/ec/dell/mec5055/Kconfig A src/ec/dell/mec5055/Makefile.inc A src/ec/dell/mec5055/early_init.c A src/ec/dell/mec5055/mec5055.c A src/ec/dell/mec5055/mec5055.h 5 files changed, 148 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44975/1
diff --git a/src/ec/dell/mec5055/Kconfig b/src/ec/dell/mec5055/Kconfig new file mode 100644 index 0000000..a7bf9a5 --- /dev/null +++ b/src/ec/dell/mec5055/Kconfig @@ -0,0 +1,4 @@ +## SPDX-License-Identifier: GPL-2.0-only + +config EC_DELL_MEC5055 + bool diff --git a/src/ec/dell/mec5055/Makefile.inc b/src/ec/dell/mec5055/Makefile.inc new file mode 100644 index 0000000..9cdfbb3 --- /dev/null +++ b/src/ec/dell/mec5055/Makefile.inc @@ -0,0 +1,8 @@ +## SPDX-License-Identifier: GPL-2.0-only + +ifeq ($(CONFIG_EC_DELL_MEC5055),y) + +bootblock-y += mec5055.c early_init.c +romstage-y += mec5055.c early_init.c + +endif diff --git a/src/ec/dell/mec5055/early_init.c b/src/ec/dell/mec5055/early_init.c new file mode 100644 index 0000000..6aec48b --- /dev/null +++ b/src/ec/dell/mec5055/early_init.c @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#include "mec5055.h" + +void mec5055_early_init(void) +{ + u8 buf[32], c; + + mec5055_ec_command_0(buf, 0xc2, 0); + mec5055_ec_command_1(0xab, NULL, 0, &c, 1); +} diff --git a/src/ec/dell/mec5055/mec5055.c b/src/ec/dell/mec5055/mec5055.c new file mode 100644 index 0000000..2deec7d --- /dev/null +++ b/src/ec/dell/mec5055/mec5055.c @@ -0,0 +1,110 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#include <arch/io.h> +#include "mec5055.h" + +static const u16 EC_CTRL = 0x910; +static const u16 EC_DATA = 0x911; + +static void wait_ec(void) +{ + u8 v; + do { + outb(0, EC_CTRL); + v = inb(EC_DATA); + } while (v != 0); +} + +static int read_ec_regs(u8 start, u8 *sdata, u8 count) +{ + if (start >= 0x20 || start + count > 0x20) + return -1; + + if (count <= 0) + return 0; + + while (count--) { + outb(start + 0x10, EC_CTRL); + *sdata = inb(EC_DATA); + sdata++; + start++; + } + + return 0; +} + +static int write_ec_regs(u8 start, const u8 *sdata, u8 count) +{ + if (start >= 0x20 || start + count > 0x20) + return -1; + + if (count <= 0) + return 0; + + while (count--) { + outb(start + 0x10, EC_CTRL); + outb(*sdata, EC_DATA); + sdata++; + start++; + } + + return 0; +} + +/* Steps to communicate with the EC: + * 1. send command: + * - write arguments to EC[2:] + * - send 0 to CTRL then cmd to DATA + * 2. wait EC + * 3. receive result: result may starts at EC[0] or EC[1] + */ + +/* send command to EC and wait, command at cmd[0], args in cmd[2:] */ +static int ec_send_command(const u8 *cmd, int argc) +{ + int result = 0; + + if (argc > 0) { + if (argc > 0x1e) + result = -1; + else + result = write_ec_regs(2, cmd + 2, argc); + } + + outb(0, EC_CTRL); + outb(cmd[0], EC_DATA); + wait_ec(); + return result; +} + +/* receive result from EC[0:] */ +int mec5055_ec_command_0(u8 *buf, u8 cmd, int argc) +{ + buf[0] = cmd; + int res = ec_send_command(buf, argc); + if (res < 0) + return res; + + return read_ec_regs(0, buf, 0x10); +} + +/* receive result from EC[1:] */ +int mec5055_ec_command_1(u8 cmd, const u8 *cmd_buf, int argc, u8 *res_buf, int res_size) +{ + if (argc >= 0x1e) + argc = 0x1e; + + if (res_size >= 0x1f) + res_size = 0x1f; + + int t = write_ec_regs(2, cmd_buf, argc); + if (t < 0) + return t; + + outb(0, EC_CTRL); + outb(cmd, EC_DATA); + wait_ec(); + + return read_ec_regs(1, res_buf, res_size); +} diff --git a/src/ec/dell/mec5055/mec5055.h b/src/ec/dell/mec5055/mec5055.h new file mode 100644 index 0000000..33c6a1d --- /dev/null +++ b/src/ec/dell/mec5055/mec5055.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#ifndef _EC_DELL_MEC5055_H_ +#define _EC_DELL_MEC5055_H_ + +#include <stddef.h> +#include <stdint.h> + +int mec5055_ec_command_0(u8 *buf, u8 cmd, int argc); +int mec5055_ec_command_1(u8 cmd, const u8 *cmd_buf, int argc, u8 *res_buf, int res_size); +void mec5055_early_init(void); + +#endif
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44975 )
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/early_i... File src/ec/dell/mec5055/early_init.c:
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/early_i... PS1, Line 2: /* This file is part of the coreboot project. */ Please remove
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... File src/ec/dell/mec5055/mec5055.h:
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 2: /* This file is part of the coreboot project. */ please remove
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... File src/ec/dell/mec5055/mec5055.c:
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 2: /* This file is part of the coreboot project. */ please remove
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44975 )
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
Patch Set 1:
(5 comments)
Very nice!
https://review.coreboot.org/c/coreboot/+/44975/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44975/1//COMMIT_MSG@10 PS1, Line 10: powered on I’d say: *being powered on* or *power-on*
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... File src/ec/dell/mec5055/mec5055.c:
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 19: int Use CB_SUCCESS and friends?
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 55: /* Steps to communicate with the EC: Please add a blank comment line:
/* * … */
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 59: * 2. wait EC Maybe: wait for EC
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 70: result = -1; Print a debug message?
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Iru Cai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44975
to look at the new patch set (#2).
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
ec: Add support for MEC5055 for Dell laptops
This EC support code is required to boot various Dell Latitude laptops, otherwise the laptop will shut down in a minute after being powered on.
Tested on Dell Latitude E7240.
Change-Id: Iee68ea52dcf0242315868a46b5e4768303e30dce Signed-off-by: Iru Cai mytbk920423@gmail.com --- A src/ec/dell/mec5055/Kconfig A src/ec/dell/mec5055/Makefile.inc A src/ec/dell/mec5055/early_init.c A src/ec/dell/mec5055/mec5055.c A src/ec/dell/mec5055/mec5055.h 5 files changed, 145 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44975/2
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44975 )
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
Patch Set 2:
I have looked at my Latitude 6430u firmware binary, disassembled a little and the code you have written looks the same for my laptop. If you address the suggestions in the comments, I can give +2
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Pablo Stebler, Iru Cai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44975
to look at the new patch set (#5).
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
ec: Add support for MEC5055 for Dell laptops
This EC support code is required to boot various Dell Latitude laptops, otherwise the laptop will shut down in a minute after being powered on.
Tested on Dell Latitude E7240.
Change-Id: Iee68ea52dcf0242315868a46b5e4768303e30dce Signed-off-by: Iru Cai mytbk920423@gmail.com --- A src/ec/dell/mec5055/Kconfig A src/ec/dell/mec5055/Makefile.inc A src/ec/dell/mec5055/early_init.c A src/ec/dell/mec5055/mec5055.c A src/ec/dell/mec5055/mec5055.h 5 files changed, 152 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/44975/5
Iru Cai (vimacs) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44975 )
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
Patch Set 5:
(8 comments)
https://review.coreboot.org/c/coreboot/+/44975/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44975/1//COMMIT_MSG@10 PS1, Line 10: powered on
I’d say: *being powered on* or *power-on*
Done
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/early_i... File src/ec/dell/mec5055/early_init.c:
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/early_i... PS1, Line 2: /* This file is part of the coreboot project. */
Please remove
Done
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... File src/ec/dell/mec5055/mec5055.h:
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 2: /* This file is part of the coreboot project. */
please remove
Done
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... File src/ec/dell/mec5055/mec5055.c:
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 2: /* This file is part of the coreboot project. */
please remove
Done
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 19: int
Use CB_SUCCESS and friends?
Done
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 55: /* Steps to communicate with the EC:
Please add a blank comment line: […]
Done
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 59: * 2. wait EC
Maybe: wait for EC
Done
https://review.coreboot.org/c/coreboot/+/44975/1/src/ec/dell/mec5055/mec5055... PS1, Line 70: result = -1;
Print a debug message?
Done
Attention is currently required from: Iru Cai (vimacs). Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44975 )
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
Patch Set 5: Code-Review+2
Attention is currently required from: Iru Cai (vimacs). Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44975 )
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
Patch Set 5:
(9 comments)
File src/ec/dell/mec5055/early_init.c:
PS5: Is it necessary to have a separate file for this one function?
https://review.coreboot.org/c/coreboot/+/44975/comment/b1723764_e2d8c94a PS5, Line 7: u8 buf[32], c; Initialise the buffer?
u8 buf[32] = { 0 };
File src/ec/dell/mec5055/mec5055.h:
https://review.coreboot.org/c/coreboot/+/44975/comment/a70a7920_2e876765 PS5, Line 6: #include <stddef.h> : #include <stdint.h> Already provided by `types.h`
File src/ec/dell/mec5055/mec5055.c:
https://review.coreboot.org/c/coreboot/+/44975/comment/e2215c4c_0db335c3 PS5, Line 4: #include <console/console.h> nit: also #include <types.h> here
https://review.coreboot.org/c/coreboot/+/44975/comment/e9b14db8_f65fc15c PS5, Line 21: 0x20 This 0x20 is the same numeric value as the `32` in early_init.c, why not #define it?
https://review.coreboot.org/c/coreboot/+/44975/comment/a88642d2_8ecb3441 PS5, Line 21: start >= 0x20 This check is redundant. Either `count` is positive (non-zero) and the other check is true, or `count` is zero and the code does nothing.
https://review.coreboot.org/c/coreboot/+/44975/comment/3e72768b_f9683ae4 PS5, Line 56: write arguments to EC[2:] I *really* don't like this kind of buffer handling. Why not have separate parameters for the command and the buffer?
https://review.coreboot.org/c/coreboot/+/44975/comment/abc34e4c_1c2ca782 PS5, Line 59: result may starts at EC[0] or EC[1] Why this difference? Does the EC handle some commands differently?
https://review.coreboot.org/c/coreboot/+/44975/comment/f9b8b8f4_9d68c812 PS5, Line 59: starts no `s` at the end: start
Attention is currently required from: Iru Cai.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44975 )
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS5: Any plans on moving this forward? Commit e0e6bccd4423 (ec/dell: Add support for the SMSC MEC5035, Change-Id Ia420cd51e9a64be5eee4af2c0d113618575522b0) adds support for SMSC MEC5035.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44975?usp=email )
Change subject: ec: Add support for MEC5055 for Dell laptops ......................................................................
Abandoned
No updates from the original author in over 2 years.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
HAOUAS Elyes (Code Review)
-
Iru Cai (vimacs) (Code Review)
-
Martin L Roth (Code Review)
-
Michał Żygowski (Code Review)
-
Paul Menzel (Code Review)