Module Name: src
Committed By: martin
Date: Sun Jul 30 12:01:54 UTC 2023
Modified Files:
src/sys/arch/amd64/conf [netbsd-10]: ALL
src/sys/arch/i386/conf [netbsd-10]: ALL
src/sys/dev/acpi [netbsd-10]: acpi_ec.c files.acpi
Log Message:
Pull up following revision(s) (requested by riastradh in ticket #259):
sys/dev/acpi/acpi_ec.c: revision 1.102
sys/dev/acpi/acpi_ec.c: revision 1.103
sys/dev/acpi/acpi_ec.c: revision 1.104
sys/dev/acpi/acpi_ec.c: revision 1.105
sys/dev/acpi/acpi_ec.c: revision 1.106
sys/dev/acpi/acpi_ec.c: revision 1.107
sys/dev/acpi/acpi_ec.c: revision 1.108
sys/dev/acpi/acpi_ec.c: revision 1.90
sys/dev/acpi/acpi_ec.c: revision 1.91
sys/dev/acpi/acpi_ec.c: revision 1.92
sys/dev/acpi/acpi_ec.c: revision 1.93
sys/dev/acpi/acpi_ec.c: revision 1.94
sys/dev/acpi/files.acpi: revision 1.128
sys/dev/acpi/acpi_ec.c: revision 1.95
sys/dev/acpi/acpi_ec.c: revision 1.96
sys/dev/acpi/acpi_ec.c: revision 1.97
sys/arch/amd64/conf/ALL: revision 1.179
sys/dev/acpi/acpi_ec.c: revision 1.98
sys/dev/acpi/acpi_ec.c: revision 1.99
sys/dev/acpi/acpi_ec.c: revision 1.87
sys/dev/acpi/acpi_ec.c: revision 1.88
sys/dev/acpi/acpi_ec.c: revision 1.89
sys/arch/i386/conf/ALL: revision 1.511
sys/dev/acpi/acpi_ec.c: revision 1.100
sys/dev/acpi/acpi_ec.c: revision 1.101
acpiec(4): Record device_t self.
Not used yet, to be used soon for device_printf and to allow making
some of the internal functions a little more type-safe later.
acpiec(4): New ACPIEC_DEBUG option.
Value is bit mask of debug messages to enable.
Enable in x86/ALL kernels.
No functional change intended when the option is off.
acpiec(4): Clarify lock order and sprinkle lock assertions.
No functional change intended.
acpiec(4): Sprinkle comments.
Note where this code is abusing cv_wait and needs a loop to handle
spurious wakeups.
No functional change intended.
acpiec(4): Assert state is free when we start a transaction.
No functional change intended.
acpiec(4): Set sc_got_sci only when a transaction is over.
Before, when the acpiec thread noticed an SCI had been requested and
entered acpiec_gpe_state_machine to send the query command, it would
see the SCI is still requested -- because it had yet to acknowledge
it by setting the query command! -- and think the EC was asking for a
_second_ SCI.
So once the first SCI transaction was over, it would start a second
one, even though the EC hadn't asked for another -- and this would
wedge on some ECs.
Now, acpiec_gpe_state_machine waits to see what state we transition
to before taking the SCI bit to mean we need to notify the acpiec
thread to handle another query.
That way, when the acpiec thread enters acpiec_gpe_state_machine with
EC_STATE_QUERY, it can send the query command first, with the side
effect of clearing the SCI bit in subsequent reads of the status
register, and it won't think another SCI has been requested until it
returns to EC_STATE_FREE and sees the SCI bit set again in the status
register.
Possibly relevant PRs:
PR kern/53135
PR kern/52763
PR kern/57162
acpiec(4): Fix cv_wait loop around sc->sc_got_sci.
That is, make it actually loop as required, so it gracefully handles
spurious wakeups instead of barging into invalid states.
acpiec(4): Fix interrupt wait loop in acpiec_gpe_query thread.
acpiec(4): Fix cv_timedwait abuse in acpiec_read/write.
acpiec(4): Don't touch sc->sc_state outside sc->sc_mtx.
acpiec(4): Merge returns in acpiec_read/write.
No functional change intended.
acpiec(4): Factor wait logic out.
No functional change intended.
acpiec(4): Pass softc, not device_t, to acpiec_gpe_state_machine.
Simpler, type-safer.
No functional change intended.
acpiec(4): Pass softc, not device_t, to acpiec_callout.
Simpler.
No functional change intended.
acpiec(4): Pass softc, not device_t, to acpiec_gpe_handler.
Simpler.
No functional change intended.
acpiec(4): Pass softc, not device_t, to acpiec_lock/unlock.
Simpler, type-safer.
No functional change intended.
acpiec(4): Pass softc, not device_t, to acpiec_read/write.
Simpler, type-safer.
No functional change intended.
acpiec(4): Pass softc, not device_t, to acpiec_gpe_query thread.
Simpler.
No functional change intended.
acpiec(4): Pass softc, not device_t, to acpiec_space_handler.
Better to keep the device_t isolated to public interfaces. Simpler
internally this way.
No functional change intended.
acpiec(4): Factor out if (state == FREE) cv_signal(sc_cv).
In principle this could have a functional change, but at worst, it is
to signal more wakeups than needed, which should always be safe.
acpiec(4): Take a lock around acpiec_cold updates.
Otherwise we race with readers -- probably harmlessly, but let's
avoid the appearance of problems.
XXX Maybe acpiec_suspend and acpiec_shutdown should interrupt
transactions and force them to fail promptly?
XXX This looks bad because acpiec_cold is global and sc->sc_mtx
doesn't look like it's global, but we expect to have only one
acpiec(4) device anyway from what I understand. Maybe we should move
acpiec_cold into the softc?
acpiec(4): One more debug message about read/write polling timeout.
To generate a diff of this commit:
cvs rdiff -u -r1.174 -r1.174.4.1 src/sys/arch/amd64/conf/ALL
cvs rdiff -u -r1.503 -r1.503.4.1 src/sys/arch/i386/conf/ALL
cvs rdiff -u -r1.86 -r1.86.4.1 src/sys/dev/acpi/acpi_ec.c
cvs rdiff -u -r1.126 -r1.126.4.1 src/sys/dev/acpi/files.acpi
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/arch/amd64/conf/ALL
diff -u src/sys/arch/amd64/conf/ALL:1.174 src/sys/arch/amd64/conf/ALL:1.174.4.1
--- src/sys/arch/amd64/conf/ALL:1.174 Sat Sep 24 11:05:17 2022
+++ src/sys/arch/amd64/conf/ALL Sun Jul 30 12:01:54 2023
@@ -1,4 +1,4 @@
-# $NetBSD: ALL,v 1.174 2022/09/24 11:05:17 riastradh Exp $
+# $NetBSD: ALL,v 1.174.4.1 2023/07/30 12:01:54 martin Exp $
# From NetBSD: GENERIC,v 1.787 2006/10/01 18:37:54 bouyer Exp
#
# ALL machine description file
@@ -17,7 +17,7 @@ include "arch/amd64/conf/std.amd64"
options INCLUDE_CONFIG_FILE # embed config file in kernel binary
-#ident "ALL-$Revision: 1.174 $"
+#ident "ALL-$Revision: 1.174.4.1 $"
maxusers 64 # estimated number of users
@@ -366,6 +366,7 @@ acpibut* at acpi? # ACPI Button
acpidalb* at acpi? # ACPI Direct Application Launch Button
acpiec* at acpi? # ACPI Embedded Controller (late)
acpiecdt* at acpi? # ACPI Embedded Controller (early)
+options ACPIEC_DEBUG=-1
acpifan* at acpi? # ACPI Fan
acpilid* at acpi? # ACPI Lid Switch
acpipmtr* at acpi? # ACPI Power Meter (experimental)
Index: src/sys/arch/i386/conf/ALL
diff -u src/sys/arch/i386/conf/ALL:1.503 src/sys/arch/i386/conf/ALL:1.503.4.1
--- src/sys/arch/i386/conf/ALL:1.503 Sat Sep 24 11:05:17 2022
+++ src/sys/arch/i386/conf/ALL Sun Jul 30 12:01:54 2023
@@ -1,4 +1,4 @@
-# $NetBSD: ALL,v 1.503 2022/09/24 11:05:17 riastradh Exp $
+# $NetBSD: ALL,v 1.503.4.1 2023/07/30 12:01:54 martin Exp $
# From NetBSD: GENERIC,v 1.787 2006/10/01 18:37:54 bouyer Exp
#
# ALL machine description file
@@ -17,7 +17,7 @@ include "arch/i386/conf/std.i386"
options INCLUDE_CONFIG_FILE # embed config file in kernel binary
-#ident "ALL-$Revision: 1.503 $"
+#ident "ALL-$Revision: 1.503.4.1 $"
maxusers 64 # estimated number of users
@@ -353,6 +353,7 @@ acpibut* at acpi? # ACPI Button
acpidalb* at acpi? # ACPI Direct Application Launch Button
acpiec* at acpi? # ACPI Embedded Controller (late)
acpiecdt* at acpi? # ACPI Embedded Controller (early)
+options ACPIEC_DEBUG=-1
acpifan* at acpi? # ACPI Fan
acpilid* at acpi? # ACPI Lid Switch
acpipmtr* at acpi? # ACPI Power Meter (experimental)
Index: src/sys/dev/acpi/acpi_ec.c
diff -u src/sys/dev/acpi/acpi_ec.c:1.86 src/sys/dev/acpi/acpi_ec.c:1.86.4.1
--- src/sys/dev/acpi/acpi_ec.c:1.86 Fri Dec 31 17:22:25 2021
+++ src/sys/dev/acpi/acpi_ec.c Sun Jul 30 12:01:53 2023
@@ -1,4 +1,4 @@
-/* $NetBSD: acpi_ec.c,v 1.86 2021/12/31 17:22:25 riastradh Exp $ */
+/* $NetBSD: acpi_ec.c,v 1.86.4.1 2023/07/30 12:01:53 martin Exp $ */
/*-
* Copyright (c) 2007 Joerg Sonnenberger <jo...@netbsd.org>.
@@ -34,14 +34,12 @@
* - read and write access from ASL, e.g. to read battery state
* - notification of ASL of System Control Interrupts.
*
- * Access to the EC is serialised by sc_access_mtx and optionally the
- * ACPI global mutex. Both locks are held until the request is fulfilled.
- * All access to the softc has to hold sc_mtx to serialise against the GPE
- * handler and the callout. sc_mtx is also used for wakeup conditions.
- *
- * SCIs are processed in a kernel thread. Handling gets a bit complicated
- * by the lock order (sc_mtx must be acquired after sc_access_mtx and the
- * ACPI global mutex).
+ * Lock order:
+ * sc_access_mtx (serializes EC transactions -- read, write, or SCI)
+ * -> ACPI global lock (excludes other ACPI access during EC transaction)
+ * -> sc_mtx (serializes state machine transitions and waits)
+ *
+ * SCIs are processed in a kernel thread.
*
* Read and write requests spin around for a short time as many requests
* can be handled instantly by the EC. During normal processing interrupt
@@ -59,7 +57,11 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: acpi_ec.c,v 1.86 2021/12/31 17:22:25 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: acpi_ec.c,v 1.86.4.1 2023/07/30 12:01:53 martin Exp $");
+
+#ifdef _KERNEL_OPT
+#include "opt_acpi_ec.h"
+#endif
#include <sys/param.h>
#include <sys/callout.h>
@@ -101,24 +103,42 @@ ACPI_MODULE_NAME ("acpi_ec")
#define EC_STATUS_SCI 0x20
#define EC_STATUS_SMI 0x40
+#define EC_STATUS_FMT \
+ "\x10\10IGN7\7SMI\6SCI\5BURST\4CMD\3IGN2\2IBF\1OBF"
+
static const struct device_compatible_entry compat_data[] = {
{ .compat = "PNP0C09" },
DEVICE_COMPAT_EOL
};
+#define EC_STATE_ENUM(F) \
+ F(EC_STATE_QUERY, "QUERY") \
+ F(EC_STATE_QUERY_VAL, "QUERY_VAL") \
+ F(EC_STATE_READ, "READ") \
+ F(EC_STATE_READ_ADDR, "READ_ADDR") \
+ F(EC_STATE_READ_VAL, "READ_VAL") \
+ F(EC_STATE_WRITE, "WRITE") \
+ F(EC_STATE_WRITE_ADDR, "WRITE_ADDR") \
+ F(EC_STATE_WRITE_VAL, "WRITE_VAL") \
+ F(EC_STATE_FREE, "FREE") \
+
enum ec_state_t {
- EC_STATE_QUERY,
- EC_STATE_QUERY_VAL,
- EC_STATE_READ,
- EC_STATE_READ_ADDR,
- EC_STATE_READ_VAL,
- EC_STATE_WRITE,
- EC_STATE_WRITE_ADDR,
- EC_STATE_WRITE_VAL,
- EC_STATE_FREE
+#define F(N, S) N,
+ EC_STATE_ENUM(F)
+#undef F
+};
+
+#ifdef ACPIEC_DEBUG
+static const char *const acpiec_state_names[] = {
+#define F(N, S) [N] = S,
+ EC_STATE_ENUM(F)
+#undef F
};
+#endif
struct acpiec_softc {
+ device_t sc_dev;
+
ACPI_HANDLE sc_ech;
ACPI_HANDLE sc_gpeh;
@@ -142,6 +162,55 @@ struct acpiec_softc {
uint8_t sc_cur_addr, sc_cur_val;
};
+#ifdef ACPIEC_DEBUG
+
+#define ACPIEC_DEBUG_ENUM(F) \
+ F(ACPIEC_DEBUG_REG, "REG") \
+ F(ACPIEC_DEBUG_RW, "RW") \
+ F(ACPIEC_DEBUG_QUERY, "QUERY") \
+ F(ACPIEC_DEBUG_TRANSITION, "TRANSITION") \
+ F(ACPIEC_DEBUG_INTR, "INTR") \
+
+enum {
+#define F(N, S) N,
+ ACPIEC_DEBUG_ENUM(F)
+#undef F
+};
+
+static const char *const acpiec_debug_names[] = {
+#define F(N, S) [N] = S,
+ ACPIEC_DEBUG_ENUM(F)
+#undef F
+};
+
+int acpiec_debug = ACPIEC_DEBUG;
+
+#define DPRINTF(n, sc, fmt, ...) do \
+{ \
+ if (acpiec_debug & __BIT(n)) { \
+ char dprintbuf[16]; \
+ const char *state; \
+ \
+ /* paranoia */ \
+ if ((sc)->sc_state < __arraycount(acpiec_state_names)) { \
+ state = acpiec_state_names[(sc)->sc_state]; \
+ } else { \
+ snprintf(dprintbuf, sizeof(dprintbuf), "0x%x", \
+ (sc)->sc_state); \
+ state = dprintbuf; \
+ } \
+ \
+ device_printf((sc)->sc_dev, "(%s) [%s] "fmt, \
+ acpiec_debug_names[n], state, ##__VA_ARGS__); \
+ } \
+} while (0)
+
+#else
+
+#define DPRINTF(n, sc, fmt, ...) __nothing
+
+#endif
+
static int acpiecdt_match(device_t, cfdata_t, void *);
static void acpiecdt_attach(device_t, device_t, void *);
@@ -166,7 +235,7 @@ static ACPI_STATUS acpiec_space_setup(AC
static ACPI_STATUS acpiec_space_handler(uint32_t, ACPI_PHYSICAL_ADDRESS,
uint32_t, ACPI_INTEGER *, void *, void *);
-static void acpiec_gpe_state_machine(device_t);
+static void acpiec_gpe_state_machine(struct acpiec_softc *);
CFATTACH_DECL_NEW(acpiec, sizeof(struct acpiec_softc),
acpiec_match, acpiec_attach, NULL, NULL);
@@ -313,6 +382,8 @@ acpiec_common_attach(device_t parent, de
ACPI_STATUS rv;
ACPI_INTEGER val;
+ sc->sc_dev = self;
+
sc->sc_csr_st = cmdt;
sc->sc_data_st = datat;
@@ -351,10 +422,10 @@ acpiec_common_attach(device_t parent, de
aprint_normal_dev(self, "using global ACPI lock\n");
callout_init(&sc->sc_pseudo_intr, CALLOUT_MPSAFE);
- callout_setfunc(&sc->sc_pseudo_intr, acpiec_callout, self);
+ callout_setfunc(&sc->sc_pseudo_intr, acpiec_callout, sc);
rv = AcpiInstallAddressSpaceHandler(sc->sc_ech, ACPI_ADR_SPACE_EC,
- acpiec_space_handler, acpiec_space_setup, self);
+ acpiec_space_handler, acpiec_space_setup, sc);
if (rv != AE_OK) {
aprint_error_dev(self,
"unable to install address space handler: %s\n",
@@ -363,7 +434,7 @@ acpiec_common_attach(device_t parent, de
}
rv = AcpiInstallGpeHandler(sc->sc_gpeh, sc->sc_gpebit,
- ACPI_GPE_EDGE_TRIGGERED, acpiec_gpe_handler, self);
+ ACPI_GPE_EDGE_TRIGGERED, acpiec_gpe_handler, sc);
if (rv != AE_OK) {
aprint_error_dev(self, "unable to install GPE handler: %s\n",
AcpiFormatException(rv));
@@ -378,7 +449,7 @@ acpiec_common_attach(device_t parent, de
}
if (kthread_create(PRI_NONE, KTHREAD_MPSAFE, NULL, acpiec_gpe_query,
- self, NULL, "acpiec sci thread")) {
+ sc, NULL, "acpiec sci thread")) {
aprint_error_dev(self, "unable to create query kthread\n");
goto post_csr_map;
}
@@ -406,8 +477,23 @@ post_data_map:
static bool
acpiec_suspend(device_t dv, const pmf_qual_t *qual)
{
+ struct acpiec_softc *sc = device_private(dv);
+ /*
+ * XXX This looks bad because acpiec_cold is global and
+ * sc->sc_mtx doesn't look like it's global, but we can have
+ * only one acpiec(4) device anyway. Maybe acpiec_cold should
+ * live in the softc to make this look less bad?
+ *
+ * XXX Should this block read/write/query transactions until
+ * resume?
+ *
+ * XXX Should this interrupt existing transactions to make them
+ * fail promptly or restart on resume?
+ */
+ mutex_enter(&sc->sc_mtx);
acpiec_cold = true;
+ mutex_exit(&sc->sc_mtx);
return true;
}
@@ -415,8 +501,11 @@ acpiec_suspend(device_t dv, const pmf_qu
static bool
acpiec_resume(device_t dv, const pmf_qual_t *qual)
{
+ struct acpiec_softc *sc = device_private(dv);
+ mutex_enter(&sc->sc_mtx);
acpiec_cold = false;
+ mutex_exit(&sc->sc_mtx);
return true;
}
@@ -424,8 +513,12 @@ acpiec_resume(device_t dv, const pmf_qua
static bool
acpiec_shutdown(device_t dv, int how)
{
+ struct acpiec_softc *sc = device_private(dv);
+ mutex_enter(&sc->sc_mtx);
acpiec_cold = true;
+ mutex_exit(&sc->sc_mtx);
+
return true;
}
@@ -491,24 +584,46 @@ acpiec_parse_gpe_package(device_t self,
static uint8_t
acpiec_read_data(struct acpiec_softc *sc)
{
- return bus_space_read_1(sc->sc_data_st, sc->sc_data_sh, 0);
+ uint8_t x;
+
+ KASSERT(mutex_owned(&sc->sc_mtx));
+
+ x = bus_space_read_1(sc->sc_data_st, sc->sc_data_sh, 0);
+ DPRINTF(ACPIEC_DEBUG_REG, sc, "read data=0x%"PRIx8"\n", x);
+
+ return x;
}
static void
acpiec_write_data(struct acpiec_softc *sc, uint8_t val)
{
+
+ KASSERT(mutex_owned(&sc->sc_mtx));
+
+ DPRINTF(ACPIEC_DEBUG_REG, sc, "write data=0x%"PRIx8"\n", val);
bus_space_write_1(sc->sc_data_st, sc->sc_data_sh, 0, val);
}
static uint8_t
acpiec_read_status(struct acpiec_softc *sc)
{
- return bus_space_read_1(sc->sc_csr_st, sc->sc_csr_sh, 0);
+ uint8_t x;
+
+ KASSERT(mutex_owned(&sc->sc_mtx));
+
+ x = bus_space_read_1(sc->sc_csr_st, sc->sc_csr_sh, 0);
+ DPRINTF(ACPIEC_DEBUG_REG, sc, "read status=0x%"PRIx8"\n", x);
+
+ return x;
}
static void
acpiec_write_command(struct acpiec_softc *sc, uint8_t cmd)
{
+
+ KASSERT(mutex_owned(&sc->sc_mtx));
+
+ DPRINTF(ACPIEC_DEBUG_REG, sc, "write command=0x%"PRIx8"\n", cmd);
bus_space_write_1(sc->sc_csr_st, sc->sc_csr_sh, 0, cmd);
}
@@ -526,9 +641,8 @@ acpiec_space_setup(ACPI_HANDLE region, u
}
static void
-acpiec_lock(device_t dv)
+acpiec_lock(struct acpiec_softc *sc)
{
- struct acpiec_softc *sc = device_private(dv);
ACPI_STATUS rv;
mutex_enter(&sc->sc_access_mtx);
@@ -537,7 +651,7 @@ acpiec_lock(device_t dv)
rv = AcpiAcquireGlobalLock(EC_LOCK_TIMEOUT,
&sc->sc_global_lock);
if (rv != AE_OK) {
- aprint_error_dev(dv,
+ aprint_error_dev(sc->sc_dev,
"failed to acquire global lock: %s\n",
AcpiFormatException(rv));
return;
@@ -546,15 +660,14 @@ acpiec_lock(device_t dv)
}
static void
-acpiec_unlock(device_t dv)
+acpiec_unlock(struct acpiec_softc *sc)
{
- struct acpiec_softc *sc = device_private(dv);
ACPI_STATUS rv;
if (sc->sc_need_global_lock) {
rv = AcpiReleaseGlobalLock(sc->sc_global_lock);
if (rv != AE_OK) {
- aprint_error_dev(dv,
+ aprint_error_dev(sc->sc_dev,
"failed to release global lock: %s\n",
AcpiFormatException(rv));
}
@@ -563,96 +676,123 @@ acpiec_unlock(device_t dv)
}
static ACPI_STATUS
-acpiec_read(device_t dv, uint8_t addr, uint8_t *val)
+acpiec_wait_timeout(struct acpiec_softc *sc)
{
- struct acpiec_softc *sc = device_private(dv);
- int i, timeo = 1000 * EC_CMD_TIMEOUT;
-
- acpiec_lock(dv);
- mutex_enter(&sc->sc_mtx);
-
- sc->sc_cur_addr = addr;
- sc->sc_state = EC_STATE_READ;
+ device_t dv = sc->sc_dev;
+ int i;
for (i = 0; i < EC_POLL_TIMEOUT; ++i) {
- acpiec_gpe_state_machine(dv);
+ acpiec_gpe_state_machine(sc);
if (sc->sc_state == EC_STATE_FREE)
- goto done;
+ return AE_OK;
delay(1);
}
+ DPRINTF(ACPIEC_DEBUG_RW, sc, "SCI polling timeout\n");
if (cold || acpiec_cold) {
+ int timeo = 1000 * EC_CMD_TIMEOUT;
+
while (sc->sc_state != EC_STATE_FREE && timeo-- > 0) {
delay(1000);
- acpiec_gpe_state_machine(dv);
+ acpiec_gpe_state_machine(sc);
}
if (sc->sc_state != EC_STATE_FREE) {
- mutex_exit(&sc->sc_mtx);
- acpiec_unlock(dv);
aprint_error_dev(dv, "command timed out, state %d\n",
sc->sc_state);
return AE_ERROR;
}
- } else if (cv_timedwait(&sc->sc_cv, &sc->sc_mtx, EC_CMD_TIMEOUT * hz)) {
- mutex_exit(&sc->sc_mtx);
- acpiec_unlock(dv);
- aprint_error_dev(dv,
- "command takes over %d sec...\n", EC_CMD_TIMEOUT);
- return AE_ERROR;
+ } else {
+ const unsigned deadline = getticks() + EC_CMD_TIMEOUT*hz;
+ unsigned delta;
+
+ while (sc->sc_state != EC_STATE_FREE &&
+ (delta = deadline - getticks()) < INT_MAX)
+ (void)cv_timedwait(&sc->sc_cv, &sc->sc_mtx, delta);
+ if (sc->sc_state != EC_STATE_FREE) {
+ aprint_error_dev(dv,
+ "command takes over %d sec...\n",
+ EC_CMD_TIMEOUT);
+ return AE_ERROR;
+ }
}
-done:
+ return AE_OK;
+}
+
+static ACPI_STATUS
+acpiec_read(struct acpiec_softc *sc, uint8_t addr, uint8_t *val)
+{
+ ACPI_STATUS rv;
+
+ acpiec_lock(sc);
+ mutex_enter(&sc->sc_mtx);
+
+ DPRINTF(ACPIEC_DEBUG_RW, sc,
+ "pid %ld %s, lid %ld%s%s: read addr 0x%"PRIx8"\n",
+ (long)curproc->p_pid, curproc->p_comm,
+ (long)curlwp->l_lid, curlwp->l_name ? " " : "",
+ curlwp->l_name ? curlwp->l_name : "",
+ addr);
+
+ KASSERT(sc->sc_state == EC_STATE_FREE);
+
+ sc->sc_cur_addr = addr;
+ sc->sc_state = EC_STATE_READ;
+
+ rv = acpiec_wait_timeout(sc);
+ if (ACPI_FAILURE(rv))
+ goto out;
+
+ DPRINTF(ACPIEC_DEBUG_RW, sc,
+ "pid %ld %s, lid %ld%s%s: read addr 0x%"PRIx8": 0x%"PRIx8"\n",
+ (long)curproc->p_pid, curproc->p_comm,
+ (long)curlwp->l_lid, curlwp->l_name ? " " : "",
+ curlwp->l_name ? curlwp->l_name : "",
+ addr, sc->sc_cur_val);
+
*val = sc->sc_cur_val;
- mutex_exit(&sc->sc_mtx);
- acpiec_unlock(dv);
- return AE_OK;
+out: mutex_exit(&sc->sc_mtx);
+ acpiec_unlock(sc);
+ return rv;
}
static ACPI_STATUS
-acpiec_write(device_t dv, uint8_t addr, uint8_t val)
+acpiec_write(struct acpiec_softc *sc, uint8_t addr, uint8_t val)
{
- struct acpiec_softc *sc = device_private(dv);
- int i, timeo = 1000 * EC_CMD_TIMEOUT;
+ ACPI_STATUS rv;
- acpiec_lock(dv);
+ acpiec_lock(sc);
mutex_enter(&sc->sc_mtx);
+ DPRINTF(ACPIEC_DEBUG_RW, sc,
+ "pid %ld %s, lid %ld%s%s write addr 0x%"PRIx8": 0x%"PRIx8"\n",
+ (long)curproc->p_pid, curproc->p_comm,
+ (long)curlwp->l_lid, curlwp->l_name ? " " : "",
+ curlwp->l_name ? curlwp->l_name : "",
+ addr, val);
+
+ KASSERT(sc->sc_state == EC_STATE_FREE);
+
sc->sc_cur_addr = addr;
sc->sc_cur_val = val;
sc->sc_state = EC_STATE_WRITE;
- for (i = 0; i < EC_POLL_TIMEOUT; ++i) {
- acpiec_gpe_state_machine(dv);
- if (sc->sc_state == EC_STATE_FREE)
- goto done;
- delay(1);
- }
+ rv = acpiec_wait_timeout(sc);
+ if (ACPI_FAILURE(rv))
+ goto out;
- if (cold || acpiec_cold) {
- while (sc->sc_state != EC_STATE_FREE && timeo-- > 0) {
- delay(1000);
- acpiec_gpe_state_machine(dv);
- }
- if (sc->sc_state != EC_STATE_FREE) {
- mutex_exit(&sc->sc_mtx);
- acpiec_unlock(dv);
- aprint_error_dev(dv, "command timed out, state %d\n",
- sc->sc_state);
- return AE_ERROR;
- }
- } else if (cv_timedwait(&sc->sc_cv, &sc->sc_mtx, EC_CMD_TIMEOUT * hz)) {
- mutex_exit(&sc->sc_mtx);
- acpiec_unlock(dv);
- aprint_error_dev(dv,
- "command takes over %d sec...\n", EC_CMD_TIMEOUT);
- return AE_ERROR;
- }
+ DPRINTF(ACPIEC_DEBUG_RW, sc,
+ "pid %ld %s, lid %ld%s%s: write addr 0x%"PRIx8": 0x%"PRIx8
+ " done\n",
+ (long)curproc->p_pid, curproc->p_comm,
+ (long)curlwp->l_lid, curlwp->l_name ? " " : "",
+ curlwp->l_name ? curlwp->l_name : "",
+ addr, val);
-done:
- mutex_exit(&sc->sc_mtx);
- acpiec_unlock(dv);
- return AE_OK;
+out: mutex_exit(&sc->sc_mtx);
+ acpiec_unlock(sc);
+ return rv;
}
/*
@@ -660,8 +800,8 @@ done:
*
* Transfer bitwidth/8 bytes of data between paddr and *value:
* from paddr to *value when func is ACPI_READ, and the other way
- * when func is ACPI_WRITE. arg is the acpiec(4) or acpiecdt(4)
- * device. region_arg is ignored (XXX why? determined by
+ * when func is ACPI_WRITE. arg is the acpiec_softc pointer.
+ * region_arg is ignored (XXX why? determined by
* acpiec_space_setup but never used by anything that I can see).
*
* The caller always provides storage at *value large enough for
@@ -691,7 +831,7 @@ static ACPI_STATUS
acpiec_space_handler(uint32_t func, ACPI_PHYSICAL_ADDRESS paddr,
uint32_t width, ACPI_INTEGER *value, void *arg, void *region_arg)
{
- device_t dv;
+ struct acpiec_softc *sc = arg;
ACPI_STATUS rv;
uint8_t addr, *buf;
unsigned int i;
@@ -701,7 +841,6 @@ acpiec_space_handler(uint32_t func, ACPI
return AE_BAD_PARAMETER;
addr = paddr;
- dv = arg;
buf = (uint8_t *)value;
rv = AE_OK;
@@ -709,7 +848,7 @@ acpiec_space_handler(uint32_t func, ACPI
switch (func) {
case ACPI_READ:
for (i = 0; i < width; i += 8, ++addr, ++buf) {
- rv = acpiec_read(dv, addr, buf);
+ rv = acpiec_read(sc, addr, buf);
if (rv != AE_OK)
break;
}
@@ -722,14 +861,15 @@ acpiec_space_handler(uint32_t func, ACPI
break;
case ACPI_WRITE:
for (i = 0; i < width; i += 8, ++addr, ++buf) {
- rv = acpiec_write(dv, addr, *buf);
+ rv = acpiec_write(sc, addr, *buf);
if (rv != AE_OK)
break;
}
break;
default:
- aprint_error("%s: invalid Address Space function called: %x\n",
- device_xname(dv), (unsigned int)func);
+ aprint_error_dev(sc->sc_dev,
+ "invalid Address Space function called: %x\n",
+ (unsigned int)func);
return AE_BAD_PARAMETER;
}
@@ -737,43 +877,71 @@ acpiec_space_handler(uint32_t func, ACPI
}
static void
+acpiec_wait(struct acpiec_softc *sc)
+{
+ int i;
+
+ /*
+ * First, attempt to get the query by polling.
+ */
+ for (i = 0; i < EC_POLL_TIMEOUT; ++i) {
+ acpiec_gpe_state_machine(sc);
+ if (sc->sc_state == EC_STATE_FREE)
+ return;
+ delay(1);
+ }
+
+ /*
+ * Polling timed out. Try waiting for interrupts -- either GPE
+ * interrupts, or periodic callouts in case GPE interrupts are
+ * broken.
+ */
+ DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI polling timeout\n");
+ while (sc->sc_state != EC_STATE_FREE)
+ cv_wait(&sc->sc_cv, &sc->sc_mtx);
+}
+
+static void
acpiec_gpe_query(void *arg)
{
- device_t dv = arg;
- struct acpiec_softc *sc = device_private(dv);
+ struct acpiec_softc *sc = arg;
uint8_t reg;
char qxx[5];
ACPI_STATUS rv;
- int i;
loop:
+ /*
+ * Wait until the EC sends an SCI requesting a query.
+ */
mutex_enter(&sc->sc_mtx);
-
- if (sc->sc_got_sci == false)
+ while (!sc->sc_got_sci)
cv_wait(&sc->sc_cv_sci, &sc->sc_mtx);
+ DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query requested\n");
mutex_exit(&sc->sc_mtx);
- acpiec_lock(dv);
+ /*
+ * EC wants to submit a query to us. Exclude concurrent reads
+ * and writes while we handle it.
+ */
+ acpiec_lock(sc);
mutex_enter(&sc->sc_mtx);
+ DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query\n");
+
+ KASSERT(sc->sc_state == EC_STATE_FREE);
+
/* The Query command can always be issued, so be defensive here. */
+ KASSERT(sc->sc_got_sci);
sc->sc_got_sci = false;
sc->sc_state = EC_STATE_QUERY;
- for (i = 0; i < EC_POLL_TIMEOUT; ++i) {
- acpiec_gpe_state_machine(dv);
- if (sc->sc_state == EC_STATE_FREE)
- goto done;
- delay(1);
- }
-
- cv_wait(&sc->sc_cv, &sc->sc_mtx);
+ acpiec_wait(sc);
-done:
reg = sc->sc_cur_val;
+ DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query: 0x%"PRIx8"\n", reg);
mutex_exit(&sc->sc_mtx);
- acpiec_unlock(dv);
+ acpiec_unlock(sc);
if (reg == 0)
goto loop; /* Spurious query result */
@@ -784,7 +952,7 @@ done:
snprintf(qxx, sizeof(qxx), "_Q%02X", (unsigned int)reg);
rv = AcpiEvaluateObject(sc->sc_ech, qxx, NULL, NULL);
if (rv != AE_OK && rv != AE_NOT_FOUND) {
- aprint_error_dev(dv, "GPE query method %s failed: %s",
+ aprint_error_dev(sc->sc_dev, "GPE query method %s failed: %s",
qxx, AcpiFormatException(rv));
}
@@ -792,15 +960,22 @@ done:
}
static void
-acpiec_gpe_state_machine(device_t dv)
+acpiec_gpe_state_machine(struct acpiec_softc *sc)
{
- struct acpiec_softc *sc = device_private(dv);
uint8_t reg;
+ KASSERT(mutex_owned(&sc->sc_mtx));
+
reg = acpiec_read_status(sc);
- if (reg & EC_STATUS_SCI)
- sc->sc_got_sci = true;
+#ifdef ACPIEC_DEBUG
+ if (acpiec_debug & __BIT(ACPIEC_DEBUG_TRANSITION)) {
+ char buf[128];
+
+ snprintb(buf, sizeof(buf), EC_STATUS_FMT, reg);
+ DPRINTF(ACPIEC_DEBUG_TRANSITION, sc, "%s\n", buf);
+ }
+#endif
switch (sc->sc_state) {
case EC_STATE_QUERY:
@@ -813,17 +988,13 @@ acpiec_gpe_state_machine(device_t dv)
case EC_STATE_QUERY_VAL:
if ((reg & EC_STATUS_OBF) == 0)
break; /* Nothing of interest here. */
-
sc->sc_cur_val = acpiec_read_data(sc);
sc->sc_state = EC_STATE_FREE;
-
- cv_signal(&sc->sc_cv);
break;
case EC_STATE_READ:
if ((reg & EC_STATUS_IBF) != 0)
break; /* Nothing of interest here. */
-
acpiec_write_command(sc, EC_COMMAND_READ);
sc->sc_state = EC_STATE_READ_ADDR;
break;
@@ -831,7 +1002,6 @@ acpiec_gpe_state_machine(device_t dv)
case EC_STATE_READ_ADDR:
if ((reg & EC_STATUS_IBF) != 0)
break; /* Nothing of interest here. */
-
acpiec_write_data(sc, sc->sc_cur_addr);
sc->sc_state = EC_STATE_READ_VAL;
break;
@@ -841,14 +1011,11 @@ acpiec_gpe_state_machine(device_t dv)
break; /* Nothing of interest here. */
sc->sc_cur_val = acpiec_read_data(sc);
sc->sc_state = EC_STATE_FREE;
-
- cv_signal(&sc->sc_cv);
break;
case EC_STATE_WRITE:
if ((reg & EC_STATUS_IBF) != 0)
break; /* Nothing of interest here. */
-
acpiec_write_command(sc, EC_COMMAND_WRITE);
sc->sc_state = EC_STATE_WRITE_ADDR;
break;
@@ -863,43 +1030,63 @@ acpiec_gpe_state_machine(device_t dv)
case EC_STATE_WRITE_VAL:
if ((reg & EC_STATUS_IBF) != 0)
break; /* Nothing of interest here. */
- sc->sc_state = EC_STATE_FREE;
- cv_signal(&sc->sc_cv);
-
acpiec_write_data(sc, sc->sc_cur_val);
+ sc->sc_state = EC_STATE_FREE;
break;
case EC_STATE_FREE:
- if (sc->sc_got_sci)
- cv_signal(&sc->sc_cv_sci);
break;
+
default:
panic("invalid state");
}
- if (sc->sc_state != EC_STATE_FREE)
+ /*
+ * If we are not in a transaction, wake anyone waiting to start
+ * one. If an SCI was requested, notify the SCI thread that it
+ * needs to handle the SCI.
+ */
+ if (sc->sc_state == EC_STATE_FREE) {
+ cv_signal(&sc->sc_cv);
+ if (reg & EC_STATUS_SCI) {
+ DPRINTF(ACPIEC_DEBUG_TRANSITION, sc,
+ "wake SCI thread\n");
+ sc->sc_got_sci = true;
+ cv_signal(&sc->sc_cv_sci);
+ }
+ }
+
+ /*
+ * In case GPE interrupts are broken, poll once per tick for EC
+ * status updates while a transaction is still pending.
+ */
+ if (sc->sc_state != EC_STATE_FREE) {
+ DPRINTF(ACPIEC_DEBUG_INTR, sc, "schedule callout\n");
callout_schedule(&sc->sc_pseudo_intr, 1);
+ }
+
+ DPRINTF(ACPIEC_DEBUG_TRANSITION, sc, "return\n");
}
static void
acpiec_callout(void *arg)
{
- device_t dv = arg;
- struct acpiec_softc *sc = device_private(dv);
+ struct acpiec_softc *sc = arg;
mutex_enter(&sc->sc_mtx);
- acpiec_gpe_state_machine(dv);
+ DPRINTF(ACPIEC_DEBUG_INTR, sc, "callout\n");
+ acpiec_gpe_state_machine(sc);
mutex_exit(&sc->sc_mtx);
}
static uint32_t
acpiec_gpe_handler(ACPI_HANDLE hdl, uint32_t gpebit, void *arg)
{
- device_t dv = arg;
- struct acpiec_softc *sc = device_private(dv);
+ struct acpiec_softc *sc = arg;
mutex_enter(&sc->sc_mtx);
- acpiec_gpe_state_machine(dv);
+ DPRINTF(ACPIEC_DEBUG_INTR, sc, "GPE\n");
+ acpiec_gpe_state_machine(sc);
mutex_exit(&sc->sc_mtx);
return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
@@ -908,13 +1095,17 @@ acpiec_gpe_handler(ACPI_HANDLE hdl, uint
ACPI_STATUS
acpiec_bus_read(device_t dv, u_int addr, ACPI_INTEGER *val, int width)
{
- return acpiec_space_handler(ACPI_READ, addr, width * 8, val, dv, NULL);
+ struct acpiec_softc *sc = device_private(dv);
+
+ return acpiec_space_handler(ACPI_READ, addr, width * 8, val, sc, NULL);
}
ACPI_STATUS
acpiec_bus_write(device_t dv, u_int addr, ACPI_INTEGER val, int width)
{
- return acpiec_space_handler(ACPI_WRITE, addr, width * 8, &val, dv,
+ struct acpiec_softc *sc = device_private(dv);
+
+ return acpiec_space_handler(ACPI_WRITE, addr, width * 8, &val, sc,
NULL);
}
Index: src/sys/dev/acpi/files.acpi
diff -u src/sys/dev/acpi/files.acpi:1.126 src/sys/dev/acpi/files.acpi:1.126.4.1
--- src/sys/dev/acpi/files.acpi:1.126 Thu May 26 02:16:44 2022
+++ src/sys/dev/acpi/files.acpi Sun Jul 30 12:01:53 2023
@@ -1,4 +1,4 @@
-# $NetBSD: files.acpi,v 1.126 2022/05/26 02:16:44 mrg Exp $
+# $NetBSD: files.acpi,v 1.126.4.1 2023/07/30 12:01:53 martin Exp $
defflag opt_acpi.h ACPIVERBOSE ACPI_DEBUG ACPI_ACTIVATE_DEV
ACPI_DSDT_OVERRIDE ACPI_SCANPCI ACPI_BREAKPOINT
@@ -45,6 +45,7 @@ device acpiec
attach acpiec at acpinodebus
device acpiecdt
attach acpiecdt at acpiecdtbus
+defparam opt_acpi_ec.h ACPIEC_DEBUG
file dev/acpi/acpi_ec.c acpiec|acpiecdt
# ACPI Lid Switch
