> Date: Wed, 24 Mar 2021 09:47:56 +0100
> From: Theo Buehler <t...@theobuehler.org>
>
> On Tue, Mar 23, 2021 at 08:29:29PM +0100, Mark Kettenis wrote:
> > > Date: Tue, 23 Mar 2021 17:39:45 +0100
> > > From: Theo Buehler <t...@theobuehler.org>
> > >
> > > On Tue, Mar 23, 2021 at 05:28:37PM +0100, Mark Kettenis wrote:
> > > > > Date: Tue, 23 Mar 2021 16:56:33 +0100
> > > > > From: Theo Buehler <t...@theobuehler.org>
> > > > >
> > > > > On Tue, Mar 23, 2021 at 04:13:53PM +0100, Mark Kettenis wrote:
> > > > > > > Date: Tue, 23 Mar 2021 14:14:40 +0100
> > > > > > > From: Theo Buehler <t...@theobuehler.org>
> > > > > >
> > > > > > It would help if you could try and boot a kernel that adds some
> > > > > > debug
> > > > > > prints instead of calling aml_die(). Probably need to know the
> > > > > > values
> > > > > > of alen, bpos, blen, mode and flag for starters.
> > > > >
> > > > > Thanks.
> > > > >
> > > > > alen 0x00, bpos 0x278, blen 0x10, mode 0x00, flag 0x605
> > > > >
> > > > > So: AML_FIELD_ACCESS(flag) == AML_FIELD_BUFFER_ACC, bpos & 0x03 == 0
> > > > > and the aml_die("Invalid GenericSerialBus access") is hit because blen
> > > > > is twice as long as it should be according to the conditional
> > > > > preceding
> > > > > it:
> > > > >
> > > > > if (AML_FIELD_ACCESS(flag) != AML_FIELD_BUFFERACC ||
> > > > > bpos & 0x3 || blen != 8)
> > > >
> > > > Right, we need to figure out what this actually means. The ACPI
> > > > standard isn't exactly clear on this...
> > > >
> > > > > If I skip the aml_die("Invalid GenericSerialBus access"), it hits the
> > > > > next aml_die("Could not find GenericSerialBus controller"); because
> > > > > node->i2c == NULL.
> > > >
> > > > If I'm reading the AML and FreeBSD dmesg correctly this is an i2c
> > > > controller that attaches to PCI. I suspect that it is dwiic(4). D
> > > > you see dwiic(4) attach?
> > >
> > > Two of them:
> > >
> > > dwiic0 at pci0 dev 21 function 0 "Intel 400 Series I2C" rev 0x00: apic 2
> > > int 22
> > > iic0 at dwiic0
> > > dwiic1 at pci0 dev 21 function 1 "Intel 400 Series I2C" rev 0x00: apic 2
> > > int 23
> > > iic1 at dwiic1
> > >
> > > > If so, the problem is that we dont't call acpi_register_gsb() for
> > > > dwiic(4) instances that attach to PCI. I'll see if I can come up with
> > > > a diff for that.
> > >
> > > Thanks a lot!
> >
> > Can you try the diff below?
>
> I tried to install a release built with this. With miniroot69.img I
> still hit the reboot loop described in my first mail:
>
> ...
> wsdisplay0 at efifb0 mux 1: console (std, vt100 emulation), using wskbd0
> aml_rwgen: unregistered RegionSpace 0x9
> Could not convert 0 to 3
>
> panic: aml_die aml_convert: 2095
> syncing disks...uvmfault(0xffffffff818e5d78, 0xbc8, 0, 1) -> e
> fatal page fault in supervisor mode
> trap type 6 code 0 rip ffffffff810e216b cs 8 rflags 10286 cr2 bc8 cpl 0 rsp
> ffff80001f76f120
> gsbase 0xffffffff818d0ff0 kgsbase 0x0
> panic trap type 6, code=0, pc=ffffffff810e216b
>
> dump to dev 17,1 not possible
> rebooting...
>
> I tried to move acpi_register_gsb() and the call to it in dwiic_pci.c
> out of SMALL_KERNEL, but that didn't help.
>
> What info would be useful to debug this further?
SMALL_KERNEL deliberately doesn't GenericSerialBus support as this
requires the presence of a whole set of I2C drivers that we have no
space for on the install media.
This machine violates the ACPI spec, since the AML should check that
the i2c bus is actually available before using it. But pointing this
out to Dell is probably not going to help.
Here is a diff that may work better. The GenericSerialBuffer stuff
allows us to propagate the status of an I2C transfer back up the
chain. So here is a diff that simply returns EIO if the controller
can't be found.
Index: dev/acpi/dsdt.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.261
diff -u -p -r1.261 dsdt.c
--- dev/acpi/dsdt.c 18 Mar 2021 00:17:26 -0000 1.261
+++ dev/acpi/dsdt.c 24 Mar 2021 16:05:29 -0000
@@ -2535,51 +2535,53 @@ aml_rwgsb(struct aml_value *conn, int al
i2c_tag_t tag;
i2c_op_t op;
i2c_addr_t addr;
- int cmdlen, buflen;
+ int cmdlen, buflen, acclen;
uint8_t cmd;
uint8_t *buf;
- int err;
+ int pos, err;
if (conn->type != AML_OBJTYPE_BUFFER || conn->length < 5 ||
AML_CRSTYPE(crs) != LR_SERBUS || AML_CRSLEN(crs) > conn->length ||
crs->lr_i2cbus.revid != 1 || crs->lr_i2cbus.type != LR_SERBUS_I2C)
aml_die("Invalid GenericSerialBus");
if (AML_FIELD_ACCESS(flag) != AML_FIELD_BUFFERACC ||
- bpos & 0x3 || blen != 8)
+ bpos & 0x3 || (blen % 8) != 0)
aml_die("Invalid GenericSerialBus access");
node = aml_searchname(conn->node,
(char *)&crs->lr_i2cbus.vdata[crs->lr_i2cbus.tlength - 6]);
- if (node == NULL || node->i2c == NULL)
- aml_die("Could not find GenericSerialBus controller");
-
switch (((flag >> 6) & 0x3)) {
case 0: /* Normal */
switch (AML_FIELD_ATTR(flag)) {
case 0x02: /* AttribQuick */
cmdlen = 0;
- buflen = 0;
+ buflen = acclen = 0;
break;
case 0x04: /* AttribSendReceive */
cmdlen = 0;
- buflen = 1;
+ acclen = 1;
+ buflen = blen / 8;
break;
case 0x06: /* AttribByte */
cmdlen = 1;
- buflen = 1;
+ acclen = 1;
+ buflen = blen / 8;
break;
case 0x08: /* AttribWord */
cmdlen = 1;
- buflen = 2;
+ acclen = 2;
+ buflen = blen / 8;
break;
case 0x0b: /* AttribBytes */
cmdlen = 1;
- buflen = alen;
+ acclen = alen;
+ buflen = blen / 8;
break;
case 0x0e: /* AttribRawBytes */
cmdlen = 0;
- buflen = alen;
+ acclen = alen;
+ buflen = blen / 8;
break;
default:
aml_die("unsupported access type 0x%x", flag);
@@ -2588,11 +2590,11 @@ aml_rwgsb(struct aml_value *conn, int al
break;
case 1: /* AttribBytes */
cmdlen = 1;
- buflen = AML_FIELD_ATTR(flag);
+ acclen = buflen = AML_FIELD_ATTR(flag);
break;
case 2: /* AttribRawBytes */
cmdlen = 0;
- buflen = AML_FIELD_ATTR(flag);
+ acclen = buflen = AML_FIELD_ATTR(flag);
break;
default:
aml_die("unsupported access type 0x%x", flag);
@@ -2606,13 +2608,32 @@ aml_rwgsb(struct aml_value *conn, int al
op = I2C_OP_WRITE_WITH_STOP;
}
+ buf = val->v_buffer;
+
+ /*
+ * Return an error if we can't find the I2C controller that
+ * we're supposed to use for this request. This shouldn't
+ * happen as AML should only make use of GenericSerialBus
+ * address space after verifying that support for it has been
+ * registered.
+ */
+ if (node == NULL || node->i2c == NULL) {
+ buf[0] = EIO;
+ return;
+ }
+
tag = node->i2c;
addr = crs->lr_i2cbus._adr;
cmd = bpos >> 3;
- buf = val->v_buffer;
iic_acquire_bus(tag, 0);
- err = iic_exec(tag, op, addr, &cmd, cmdlen, &buf[2], buflen, 0);
+ for (pos = 0; pos < buflen; pos += acclen) {
+ err = iic_exec(tag, op, addr, &cmd, cmdlen,
+ &buf[pos + 2], acclen, 0);
+ if (err)
+ break;
+ cmd++;
+ }
iic_release_bus(tag, 0);
/*
Index: dev/pci/dwiic_pci.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/dwiic_pci.c,v
retrieving revision 1.15
diff -u -p -r1.15 dwiic_pci.c
--- dev/pci/dwiic_pci.c 25 Dec 2020 21:48:27 -0000 1.15
+++ dev/pci/dwiic_pci.c 24 Mar 2021 16:05:29 -0000
@@ -225,7 +225,12 @@ dwiic_pci_attach(struct device *parent,
config_found((struct device *)sc, &sc->sc_iba, iicbus_print);
- return;
+#if NACPI > 0 && !defined(SMALL_KERNEL)
+ if (sc->sc_devnode) {
+ sc->sc_devnode->i2c = &sc->sc_i2c_tag;
+ acpi_register_gsb(acpi_softc, sc->sc_devnode);
+ }
+#endif
}
int
