James has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31382
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
ec/quanta/it8518: Use ACPI EC code
ec/quanta/it8518 implements its own code for reading and writing the EC. As a result, it cannot be used with ec/acpi. ec/quanta/it8518 contains definitions that are useful for mb/thinkpad/x131e, however, x131e uses ec/acpi.
This replaces functions and definitions in ec/quanta/it8518 with their equivalents in ec/acpi.
TODO: test on google/stout
Change-Id: I7bea7445f34817cef1602843bbded59230bb8d47 Signed-off-by: James Ye jye836@gmail.com --- M src/ec/quanta/it8518/Kconfig M src/ec/quanta/it8518/ec.c M src/ec/quanta/it8518/ec.h M src/mainboard/google/stout/chromeos.c M src/mainboard/google/stout/ec.c M src/mainboard/google/stout/mainboard.c M src/mainboard/google/stout/mainboard_smi.c M src/mainboard/google/stout/romstage.c 8 files changed, 17 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31382/1
diff --git a/src/ec/quanta/it8518/Kconfig b/src/ec/quanta/it8518/Kconfig index 24ac36f..caaccd5 100644 --- a/src/ec/quanta/it8518/Kconfig +++ b/src/ec/quanta/it8518/Kconfig @@ -1,4 +1,5 @@ config EC_QUANTA_IT8518 + select EC_ACPI bool help Interface to QUANTA IT8518 Embedded Controller. diff --git a/src/ec/quanta/it8518/ec.c b/src/ec/quanta/it8518/ec.c index e293f7c..a27e7b9 100644 --- a/src/ec/quanta/it8518/ec.c +++ b/src/ec/quanta/it8518/ec.c @@ -19,6 +19,7 @@ #include <delay.h> #include <device/device.h> #include <device/pnp.h> +#include <ec/acpi/ec.h> #include <pc80/keyboard.h> #include <stdlib.h>
@@ -93,48 +94,16 @@ * These functions are for accessing the IT8518 device RAM space via 0x66/0x68 */
-u8 ec_read_ob(void) -{ - if (!output_buffer_full(EC_SC)) return 0; - return inb(EC_DATA); -} - -void ec_write_cmd(u8 cmd) -{ - if (!input_buffer_empty(EC_SC)) return; - outb(cmd, EC_SC); -} - -void ec_write_ib(u8 data) -{ - if (!input_buffer_empty(EC_SC)) return; - outb(data, EC_DATA); -} - -u8 ec_read(u16 addr) -{ - ec_write_cmd(RD_EC); - ec_write_ib(addr); - return ec_read_ob(); -} - -void ec_write(u16 addr, u8 data) -{ - ec_write_cmd(WR_EC); - ec_write_ib(addr); - ec_write_ib(data); -} - #ifndef __PRE_RAM__
u8 ec_it8518_get_event(void) { u8 cmd = 0; u8 status = inb(EC_SC); - if (status & SCI_EVT) { - ec_write_cmd(QR_EC); - cmd = ec_read_ob(); - } else if (status & SMI_EVT) { + if (status & EC_SCI_EVT) { + send_ec_command(QR_EC); + cmd = recv_ec_data(); + } else if (status & EC_SMI_EVT) { ec_kbc_write_cmd(EC_KBD_SMI_EVENT); cmd = ec_kbc_read_ob(); } diff --git a/src/ec/quanta/it8518/ec.h b/src/ec/quanta/it8518/ec.h index 6fca3b9..27f9137 100644 --- a/src/ec/quanta/it8518/ec.h +++ b/src/ec/quanta/it8518/ec.h @@ -43,17 +43,6 @@ void ec_kbc_write_cmd(u8 cmd); void ec_kbc_write_ib(u8 data);
-// 62h/66h Command Interface -#define EC_DATA 0x62 -#define EC_SC 0x66 // Status & Control Register -#define SMI_EVT (1 << 6) // 1: SMI event was triggered -#define SCI_EVT (1 << 5) // 1: SCI event was triggered - -// EC Commands (defined in ec_function_spec v3.12) -#define RD_EC 0x80 -#define WR_EC 0x81 -#define QR_EC 0x84 - #define EC_CMD_EXIT_BOOT_BLOCK 0x85 #define EC_CMD_NOTIFY_ACPI_ENTER 0x86 #define EC_CMD_NOTIFY_ACPI_EXIT 0x87 @@ -63,13 +52,13 @@ #define EC_PERIPH_CNTL_3 0x0D #define EC_USB_S3_EN 0x26 #define EC_PERIPH_STAT_3 0x35 +#define EC_MBAT_STATUS 0x38 #define EC_THERM_0 0x78 #define EC_WAKE_SRC_ENABLE 0xBF #define EC_FW_VER 0xE8 // 2 Bytes #define EC_IF_MIN_VER 0xEB #define EC_STATUS_REG 0xEC #define EC_IF_MAJ_VER 0xEF -#define EC_MBAT_STATUS 0x0138
// EC 0.83b added status bits: @@ -81,12 +70,6 @@ // EC 0.86a added enable bit: #define EC_LID_WAKE_ENABLE 0x4
-u8 ec_read_ob(void); -void ec_write_cmd(u8 cmd); -void ec_write_ib(u8 data); - -u8 ec_read(u16 addr); -void ec_write(u16 addr, u8 data); u8 ec_it8518_get_event(void); void ec_it8518_enable_wake_events(void);
diff --git a/src/mainboard/google/stout/chromeos.c b/src/mainboard/google/stout/chromeos.c index 5301d30..f7f54c1 100644 --- a/src/mainboard/google/stout/chromeos.c +++ b/src/mainboard/google/stout/chromeos.c @@ -24,6 +24,7 @@ #include <southbridge/intel/common/gpio.h> #include <vendorcode/google/chromeos/chromeos.h> #include "ec.h" +#include <ec/acpi/ec.h> #include <ec/quanta/it8518/ec.h>
#ifndef __PRE_RAM__ diff --git a/src/mainboard/google/stout/ec.c b/src/mainboard/google/stout/ec.c index 229f919..11cdc2a 100644 --- a/src/mainboard/google/stout/ec.c +++ b/src/mainboard/google/stout/ec.c @@ -18,6 +18,7 @@ #include <bootmode.h> #include <types.h> #include <console/console.h> +#include <ec/acpi/ec.h> #include <ec/quanta/it8518/ec.h> #include <device/device.h> #include <device/pci.h> diff --git a/src/mainboard/google/stout/mainboard.c b/src/mainboard/google/stout/mainboard.c index b63083f..63cc67b 100644 --- a/src/mainboard/google/stout/mainboard.c +++ b/src/mainboard/google/stout/mainboard.c @@ -29,13 +29,14 @@ #include <southbridge/intel/bd82x6x/pch.h> #include <smbios.h> #include <device/pci.h> +#include <ec/acpi/ec.h> #include <ec/quanta/it8518/ec.h> #include <vendorcode/google/chromeos/chromeos.h>
void mainboard_suspend_resume(void) { /* Stout EC needs to be put back in ACPI mode */ - ec_write_cmd(EC_CMD_NOTIFY_ACPI_ENTER); + send_ec_command(EC_CMD_NOTIFY_ACPI_ENTER); }
diff --git a/src/mainboard/google/stout/mainboard_smi.c b/src/mainboard/google/stout/mainboard_smi.c index 6a51645..27eae19 100644 --- a/src/mainboard/google/stout/mainboard_smi.c +++ b/src/mainboard/google/stout/mainboard_smi.c @@ -24,6 +24,7 @@ #include <cpu/intel/model_206ax/model_206ax.h>
/* Include EC functions */ +#include <ec/acpi/ec.h> #include <ec/quanta/it8518/ec.h> #include "ec.h"
@@ -96,13 +97,13 @@ /* * TODO(kimarie) Clear all pending events and enable SCI. */ - ec_write_cmd(EC_CMD_NOTIFY_ACPI_ENTER); + send_ec_command(EC_CMD_NOTIFY_ACPI_ENTER); break; case APMC_ACPI_DIS: /* * TODO(kimarie) Clear all pending events and enable SMI. */ - ec_write_cmd(EC_CMD_NOTIFY_ACPI_EXIT); + send_ec_command(EC_CMD_NOTIFY_ACPI_EXIT); break; } return 0; diff --git a/src/mainboard/google/stout/romstage.c b/src/mainboard/google/stout/romstage.c index d212951..66eb953 100644 --- a/src/mainboard/google/stout/romstage.c +++ b/src/mainboard/google/stout/romstage.c @@ -29,6 +29,7 @@ #include <southbridge/intel/common/gpio.h> #include <halt.h> #include <bootmode.h> +#include <ec/acpi/ec.h> #include <ec/quanta/it8518/ec.h> #include "ec.h" #include "onboard.h" @@ -121,12 +122,12 @@ * Tell EC to exit RO mode */ printk(BIOS_DEBUG, "EC will exit RO mode and boot normally\n"); - ec_write_cmd(EC_CMD_EXIT_BOOT_BLOCK); + send_ec_command(EC_CMD_EXIT_BOOT_BLOCK); die("wait for ec to reset"); } } else { printk(BIOS_DEBUG, "EC Warm Boot Detected\n"); - ec_write_cmd(EC_CMD_WARM_RESET); + send_ec_command(EC_CMD_WARM_RESET); } }
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31382 )
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
Patch Set 1:
any specific tests you'd like me to do on Stout? I cherry picked this and everything EC-related (battery, kb, mouse) seemed to work ok under both Windows and Linux
James has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31382 )
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
Patch Set 1:
Patch Set 1:
any specific tests you'd like me to do on Stout? I cherry picked this and everything EC-related (battery, kb, mouse) seemed to work ok under both Windows and Linux
Are you able to specifically test that the "input buffer full" and "output buffer empty" situations behave as expected? I'm not sure how to do this.
I'm reasonably confident it should do the same thing (the timeout is slightly different), but it would be good to specifically test it.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31382 )
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
Patch Set 1:
Are you able to specifically test that the "input buffer full" and "output buffer empty" situations behave as expected? I'm not sure how to do this.
I'm not sure either, outside of forcibly doing so. It's unclear how those conditions would occur during normal usage
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Matt DeVillier, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31382
to look at the new patch set (#2).
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
ec/quanta/it8518: Use ACPI EC code
ec/quanta/it8518 contains code for reading and writing the EC. lenovo/x131e uses the Quanta IT8518, but with the H8 EC code in coreboot. To mix includes from ec/quanta/it8518 with code from H8, drop duplicate functions and definitions from quanta/it8518 and use code from ec/acpi instead.
Change-Id: I7bea7445f34817cef1602843bbded59230bb8d47 Signed-off-by: James Ye jye836@gmail.com --- M src/ec/quanta/it8518/Kconfig M src/ec/quanta/it8518/ec.c M src/ec/quanta/it8518/ec.h M src/mainboard/google/stout/chromeos.c M src/mainboard/google/stout/early_init.c M src/mainboard/google/stout/ec.c M src/mainboard/google/stout/mainboard.c M src/mainboard/google/stout/mainboard_smi.c 8 files changed, 16 insertions(+), 58 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31382/2
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Matt DeVillier, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31382
to look at the new patch set (#3).
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
ec/quanta/it8518: Use ACPI EC code
ec/quanta/it8518 contains code for reading and writing the EC. lenovo/x131e uses the Quanta IT8518, but with the H8 EC code in coreboot. To mix includes from ec/quanta/it8518 with code from H8, drop duplicate functions and definitions from quanta/it8518 and use code from ec/acpi instead.
Also fix a bad constant MBAT_STATUS.
Change-Id: I7bea7445f34817cef1602843bbded59230bb8d47 Signed-off-by: James Ye jye836@gmail.com --- M src/ec/quanta/it8518/Kconfig M src/ec/quanta/it8518/ec.c M src/ec/quanta/it8518/ec.h M src/mainboard/google/stout/chromeos.c M src/mainboard/google/stout/early_init.c M src/mainboard/google/stout/ec.c M src/mainboard/google/stout/mainboard.c M src/mainboard/google/stout/mainboard_smi.c 8 files changed, 17 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31382/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31382 )
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31382/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31382/3//COMMIT_MSG@15 PS3, Line 15: : Also fix a bad constant MBAT_STATUS. Why is it "bad"? Why is it done in this change?
James has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31382 )
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
Patch Set 3:
Patch Set 3:
(1 comment)
Previously the constant was truncated to u8 in `ec/quanta/it8518/ec.c:ec_read()`. That's replaced with `ec/acpi/ec.c:ec_read()`, and a compiler warning causes the stout build to fail (e.g. previous failed Jenkins build).
I don't know why it was 0x0138 before (maybe a typo + automatic formatting?), but 0x38 is the correct ECRAM address.
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Matt DeVillier, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31382
to look at the new patch set (#4).
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
ec/quanta/it8518: Use ACPI EC code
ec/quanta/it8518 contains code for reading and writing the EC. lenovo/x131e uses the Quanta IT8518, but with the H8 EC code in coreboot. To mix includes from ec/quanta/it8518 with code from H8, drop duplicate functions and definitions from quanta/it8518 and use code from ec/acpi instead.
Also fix an overflowing constant MBAT_STATUS causing build failures.
Change-Id: I7bea7445f34817cef1602843bbded59230bb8d47 Signed-off-by: James Ye jye836@gmail.com --- M src/ec/quanta/it8518/Kconfig M src/ec/quanta/it8518/ec.c M src/ec/quanta/it8518/ec.h M src/mainboard/google/stout/chromeos.c M src/mainboard/google/stout/early_init.c M src/mainboard/google/stout/ec.c M src/mainboard/google/stout/mainboard.c M src/mainboard/google/stout/mainboard_smi.c 8 files changed, 17 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31382/4
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31382?usp=email )
Change subject: ec/quanta/it8518: Use ACPI EC code ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
James (Code Review)
-
Martin L Roth (Code Review)
-
Matt DeVillier (Code Review)