The asmc(4) driver is a bit slow. On my MacBookPro12,1 it takes a
couple of minutes to probe all the sensors. And changing the keyboard
backlight takes a couple of seconds. This is especially annoying when
you're writing a diff to bind that functionality to the keys reserved
for this purpose on the keyboard.
Turns out the chip used for the Apple SMC is my old friend the H8S
microcontroller (see lom(4) on sparc64). With that knowledge in hand,
it was fairly obvious what was going wrong. The driver didn't check
whether the chip input buffer was full before writing to it. That
caused commands to get lost. With that fixed, comunication with the
chip seems to be reliable, but still somewhat slow. There were two
reasons for this slowness:
1. The asmc_command() function does additional reads as a "sanity
flush". We had to wait for these reads to time out, which took a
significant fraction of a second. On my system, I neversaw any of
these flusing reads succeed. So I removed that bit of code.
2. Failing commands were retried up to 10 times. I assume this was
necessary because writing to the chip was unreliable. I reduce it
to 3 retries.
3. The timouts for individual reads and writes were quite high.
Especially because the code used tsleep(), which means a context
switch could happen and we'd sleep longer than intended.
Since I also noticed that the command to set the keyboard backlight
succeeded initially (when a simple delay was used) but failed later
(when tsleep was used), I concluded that the SMC times out as well if
we didn't send the complete command within a certain time. Such a
minimum time is hard to guarantee if we use tsleep. So this
driverreally has to use an actual (spinning) delay, just like what I
did in my lom(4) driver.
That sounds worse than it is. Typically we spin only for a short time
(500 us seems to be typical) for read and write operations that
succeed. So I clamped it at 5 ms. With the diff below, the asmc
taskq consumes about the same amount of CPU as the acpi thread. I
think that is perfectly acceptable.
However, since Apple's SMC implementation almost certainly changed
over the years, it would be good to test this diff on a wider range of
Apple hardware. Please check that all sensors are still present and
are properly updated after applying this diff.
Thanks,
Mark
Index: asmc.c
===================================================================
RCS file: /cvs/src/sys/dev/isa/asmc.c,v
retrieving revision 1.21
diff -u -p -r1.21 asmc.c
--- asmc.c 15 Dec 2015 20:58:22 -0000 1.21
+++ asmc.c 21 Dec 2015 21:35:05 -0000
@@ -44,7 +44,11 @@
#define ASMC_WRITE 0x11 /* SMC write command */
#define ASMC_INFO 0x13 /* SMC info/type command */
-#define ASMC_RETRY 10
+#define ASMC_OBF 0x01 /* Output buffer full */
+#define ASMC_IBF 0x02 /* Input buffer full */
+#define ASMC_ACCEPT 0x04
+
+#define ASMC_RETRY 3
#define ASMC_MAXLEN 32 /* SMC maximum data size len */
#define ASMC_NOTFOUND 0x84 /* SMC status key not found */
@@ -385,11 +389,8 @@ asmc_kbdled(void *arg)
int r;
if ((r = asmc_try(sc, ASMC_WRITE, "LKSB", buf, 2)))
-#if 0 /* todo: write error, as no 0x04 wait status, but led is changed */
printf("%s: keyboard backlight failed (0x%x)\n",
sc->sc_dev.dv_xname, r);
-#endif
- ;
}
int
@@ -436,44 +437,57 @@ asmc_status(struct asmc_softc *sc)
}
static int
-asmc_wait(struct asmc_softc *sc, uint8_t m)
+asmc_write(struct asmc_softc *sc, uint8_t off, uint8_t val)
{
- int us;
+ uint8_t str;
+ int i;
- for (us = (2 << 4); us < (2 << 16); us *= 2) { /* wait up to 128 ms */
- (!sc->sc_sensor_task) ? delay(us) :
- tsleep(&sc->sc_taskq, 0, "asmc",
- (us * hz + 999999) / 1000000 + 1);
- if (bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASMC_COMMAND) & m)
- return 1;
+ for (i = 500; i > 0; i--) {
+ str = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASMC_COMMAND);
+ if ((str & ASMC_IBF) == 0)
+ break;
+ delay(10);
}
- return 0;
-}
+ if (i == 0)
+ return ETIMEDOUT;
-static int
-asmc_write(struct asmc_softc *sc, uint8_t off, uint8_t val)
-{
bus_space_write_1(sc->sc_iot, sc->sc_ioh, off, val);
- if (asmc_wait(sc, 0x04)) /* write accepted? */
- return 0;
- return 1;
+
+ for (i = 500; i > 0; i--) {
+ str = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASMC_COMMAND);
+ if (str & ASMC_ACCEPT)
+ break;
+ delay(10);
+ }
+ if (i == 0)
+ return ETIMEDOUT;
+
+ return 0;
}
static int
asmc_read(struct asmc_softc *sc, uint8_t off, uint8_t *buf)
{
- if (asmc_wait(sc, 0x01)) { /* ready for read? */
- *buf = bus_space_read_1(sc->sc_iot, sc->sc_ioh, off);
- return 0;
+ uint8_t str;
+ int i;
+
+ for (i = 500; i > 0; i--) {
+ str = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASMC_COMMAND);
+ if (str & ASMC_OBF)
+ break;
+ delay(10);
}
- return 1;
+ if (i == 0)
+ return ETIMEDOUT;
+
+ *buf = bus_space_read_1(sc->sc_iot, sc->sc_ioh, off);
+ return 0;
}
static int
asmc_command(struct asmc_softc *sc, int cmd, const char *key, uint8_t *buf,
uint8_t len)
{
- uint8_t n;
int i;
if (len > ASMC_MAXLEN)
@@ -489,9 +503,6 @@ asmc_command(struct asmc_softc *sc, int
for (i = 0; i < len; i++)
if (asmc_read(sc, ASMC_DATA, &buf[i]))
return 1;
- for (i = 0; i < ASMC_MAXLEN; i++) /* sanity flush */
- if (asmc_read(sc, ASMC_DATA, &n))
- break;
} else if (cmd == ASMC_WRITE) {
for (i = 0; i < len; i++)
if (asmc_write(sc, ASMC_DATA, buf[i]))
@@ -515,9 +526,7 @@ asmc_try(struct asmc_softc *sc, int cmd,
if (r && (s = asmc_status(sc)))
r = s;
rw_exit_write(&sc->sc_lock);
- if (sc->sc_sensor_task) /* give kbdled a chance to grab the lock */
- tsleep(&sc->sc_taskq, 0, "asmc",
- (1 * hz + 999999) / 1000000 + 1);
+
return r;
}
