> Date: Fri, 21 Oct 2016 10:37:08 +0300 > From: Paul Irofti <p...@irofti.net> > > On Tue, Oct 18, 2016 at 09:01:59PM -0700, Ilya Kaliman wrote: > > Thanks! Now everything seems to work. Minor tweak - maybe need extra > > newline after "acpiec0 at acpi0" > > New diff that fixes the printfs, removes dead code (suggested by > guenther@) and takes care of some small style(9) nits. OK?
I'm not really happy about the use of the static variable in the resource parsing function. It feels like the aml_parse_resource API should be changed to pass the resource number to the callback function. > Index: acpidev.h > =================================================================== > RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v > retrieving revision 1.38 > diff -u -p -u -p -r1.38 acpidev.h > --- acpidev.h 12 Aug 2015 05:59:54 -0000 1.38 > +++ acpidev.h 21 Oct 2016 07:35:22 -0000 > @@ -323,10 +323,12 @@ struct acpiec_softc { > int sc_ecbusy; > > /* command/status register */ > + bus_size_t sc_ec_sc; > bus_space_tag_t sc_cmd_bt; > bus_space_handle_t sc_cmd_bh; > > /* data register */ > + bus_size_t sc_ec_data; > bus_space_tag_t sc_data_bt; > bus_space_handle_t sc_data_bh; > > Index: acpiec.c > =================================================================== > RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v > retrieving revision 1.54 > diff -u -p -u -p -r1.54 acpiec.c > --- acpiec.c 23 Aug 2016 18:26:21 -0000 1.54 > +++ acpiec.c 21 Oct 2016 07:35:22 -0000 > @@ -48,7 +48,7 @@ void acpiec_write(struct acpiec_softc * > > int acpiec_getcrs(struct acpiec_softc *, > struct acpi_attach_args *); > -int acpiec_getregister(const u_int8_t *, int, int *, bus_size_t *); > +int acpiec_parse_resources(union acpi_resource *, void *); > > void acpiec_wait(struct acpiec_softc *, u_int8_t, u_int8_t); > void acpiec_sci_event(struct acpiec_softc *); > @@ -285,15 +285,16 @@ acpiec_attach(struct device *parent, str > return; > } > > + printf("\n"); > if (acpiec_getcrs(sc, aa)) { > - printf(": Failed to read resource settings\n"); > + printf("%s: Failed to read resource settings\n", DEVNAME(sc)); > return; > } > > sc->sc_acpi->sc_ec = sc; > > if (acpiec_reg(sc)) { > - printf(": Failed to register address space\n"); > + printf("%s: Failed to register address space\n", DEVNAME(sc)); > return; > } > > @@ -305,15 +306,13 @@ acpiec_attach(struct device *parent, str > acpi_set_gpehandler(sc->sc_acpi, sc->sc_gpe, acpiec_gpehandler, > sc, 1); > #endif > - > + > if (aml_evalname(sc->sc_acpi, sc->sc_devnode, "_GLK", 0, NULL, &res)) > sc->sc_glk = 0; > else if (res.type != AML_OBJTYPE_INTEGER) > sc->sc_glk = 0; > else > sc->sc_glk = res.v_integer ? 1 : 0; > - > - printf("\n"); > } > > void > @@ -366,68 +365,75 @@ acpiec_gpehandler(struct acpi_softc *acp > return (0); > } > > -/* parse the resource buffer to get a 'register' value */ > int > -acpiec_getregister(const u_int8_t *buf, int size, int *type, bus_size_t > *addr) > +acpiec_parse_resources(union acpi_resource *crs, void *arg) > { > - int len, hlen; > + struct acpiec_softc *sc = arg; > + int type = AML_CRSTYPE(crs); > > -#define RES_TYPE_MASK 0x80 > -#define RES_LENGTH_MASK 0x07 > -#define RES_TYPE_IOPORT 0x47 > -#define RES_TYPE_ENDTAG 0x79 > + static int argno = 0; > > - if (size <= 0) > - return (0); > - > - if (*buf & RES_TYPE_MASK) { > - /* large resource */ > - if (size < 3) > - return (1); > - len = (int)buf[1] + 256 * (int)buf[2]; > - hlen = 3; > - } else { > - /* small resource */ > - len = buf[0] & RES_LENGTH_MASK; > - hlen = 1; > + switch (argno) { > + case 0: > + if (type != SR_IOPORT) { > + printf("%s: Unexpected resource #%d type %d\n", > + DEVNAME(sc), argno, type); > + break; > + } > + sc->sc_data_bt = sc->sc_acpi->sc_iot; > + sc->sc_ec_data = crs->sr_ioport._max; > + break; > + case 1: > + if (type != SR_IOPORT) { > + printf("%s: Unexpected resource #%d type %d\n", > + DEVNAME(sc), argno, type); > + break; > + } > + sc->sc_cmd_bt = sc->sc_acpi->sc_iot; > + sc->sc_ec_sc = crs->sr_ioport._max; > + break; > + case 2: > + if (!sc->sc_acpi->sc_hw_reduced) { > + printf("%s: Not running on HW-Reduced ACPI type %d\n", > + DEVNAME(sc), type); > + break; > + } > + /* XXX: handle SCI GPIO */ > + break; > + default: > + printf("%s: invalid resource #%d type %d\n", > + DEVNAME(sc), argno, type); > } > > - /* XXX todo: decode other types */ > - if (*buf != RES_TYPE_IOPORT) > - return (0); > - > - if (size < hlen + len) > - return (0); > - > - /* XXX validate? */ > - *type = GAS_SYSTEM_IOSPACE; > - *addr = (int)buf[2] + 256 * (int)buf[3]; > - > - return (hlen + len); > + argno++; > + return 0; > } > > int > acpiec_getcrs(struct acpiec_softc *sc, struct acpi_attach_args *aa) > { > struct aml_value res; > - bus_size_t ec_sc, ec_data; > - int dtype, ctype; > - char *buf; > - int size, ret; > int64_t gpe; > struct acpi_ecdt *ecdt = aa->aaa_table; > extern struct aml_node aml_root; > + int rc; > > /* Check if this is ECDT initialization */ > if (ecdt) { > /* Get GPE, Data and Control segments */ > sc->sc_gpe = ecdt->gpe_bit; > > - ctype = ecdt->ec_control.address_space_id; > - ec_sc = ecdt->ec_control.address; > + if (ecdt->ec_control.address_space_id == GAS_SYSTEM_IOSPACE) > + sc->sc_cmd_bt = sc->sc_acpi->sc_iot; > + else > + sc->sc_cmd_bt = sc->sc_acpi->sc_memt; > + sc->sc_ec_sc = ecdt->ec_control.address; > > - dtype = ecdt->ec_data.address_space_id; > - ec_data = ecdt->ec_data.address; > + if (ecdt->ec_data.address_space_id == GAS_SYSTEM_IOSPACE) > + sc->sc_data_bt = sc->sc_acpi->sc_iot; > + else > + sc->sc_data_bt = sc->sc_acpi->sc_memt; > + sc->sc_ec_data = ecdt->ec_data.address; > > /* Get devnode from header */ > sc->sc_devnode = aml_searchname(&aml_root, ecdt->ec_id); > @@ -435,7 +441,9 @@ acpiec_getcrs(struct acpiec_softc *sc, s > goto ecdtdone; > } > > - if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "_GPE", 0, NULL, > &gpe)) { > + rc = aml_evalinteger(sc->sc_acpi, sc->sc_devnode, > + "_GPE", 0, NULL, &gpe); > + if (rc) { > dnprintf(10, "%s: no _GPE\n", DEVNAME(sc)); > return (1); > } > @@ -456,60 +464,26 @@ acpiec_getcrs(struct acpiec_softc *sc, s > return (1); > } > > - size = res.length; > - buf = res.v_buffer; > - > - ret = acpiec_getregister(buf, size, &dtype, &ec_data); > - if (ret <= 0) { > - dnprintf(10, "%s: failed to read DATA from _CRS\n", > - DEVNAME(sc)); > - aml_freevalue(&res); > - return (1); > - } > - > - buf += ret; > - size -= ret; > - > - ret = acpiec_getregister(buf, size, &ctype, &ec_sc); > - if (ret <= 0) { > - dnprintf(10, "%s: failed to read S/C from _CRS\n", > - DEVNAME(sc)); > - aml_freevalue(&res); > - return (1); > - } > - > - buf += ret; > - size -= ret; > - > - if (size != 2 || *buf != RES_TYPE_ENDTAG) { > - dnprintf(10, "%s: no _CRS end tag\n", DEVNAME(sc)); > - aml_freevalue(&res); > + aml_parse_resource(&res, acpiec_parse_resources, sc); > + aml_freevalue(&res); > + if (sc->sc_ec_data == 0 || sc->sc_ec_sc == 0) { > + printf("%s: failed to read from _CRS\n", DEVNAME(sc)); > return (1); > } > - aml_freevalue(&res); > > - /* XXX: todo - validate _CRS checksum? */ > ecdtdone: > > dnprintf(10, "%s: Data: 0x%lx, S/C: 0x%lx\n", > - DEVNAME(sc), ec_data, ec_sc); > + DEVNAME(sc), sc->sc_ec_data, sc->sc_ec_sc); > > - if (ctype == GAS_SYSTEM_IOSPACE) > - sc->sc_cmd_bt = aa->aaa_iot; > - else > - sc->sc_cmd_bt = aa->aaa_memt; > - > - if (bus_space_map(sc->sc_cmd_bt, ec_sc, 1, 0, &sc->sc_cmd_bh)) { > + if (bus_space_map(sc->sc_cmd_bt, sc->sc_ec_sc, 1, 0, &sc->sc_cmd_bh)) { > dnprintf(10, "%s: failed to map S/C reg.\n", DEVNAME(sc)); > return (1); > } > > - if (dtype == GAS_SYSTEM_IOSPACE) > - sc->sc_data_bt = aa->aaa_iot; > - else > - sc->sc_data_bt = aa->aaa_memt; > - > - if (bus_space_map(sc->sc_data_bt, ec_data, 1, 0, &sc->sc_data_bh)) { > + rc = bus_space_map(sc->sc_data_bt, sc->sc_ec_data, 1, 0, > + &sc->sc_data_bh); > + if (rc) { > dnprintf(10, "%s: failed to map DATA reg.\n", DEVNAME(sc)); > bus_space_unmap(sc->sc_cmd_bt, sc->sc_cmd_bh, 1); > return (1); > >