| Message ID | 1478019507.1676.23.camel@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Nov 1, 2016 at 5:58 PM, Jérôme de Bretagne <jerome.debretagne@gmail.com> wrote: > Hi Heikki, Andy, Greg, Rafael, > > I've detected a regression on my Lenovo ThinkPad 8 tablet introduced during > 4.9-rc1, preventing the built-in Bluetooth Broadcom chipset from properly > initializing, with many timeout messages in dmesg like these: > "serial8250_interrupt: WXYZ callbacks suppressed" > "serial8250: too much work for irq39" > cf. my earlier report on linux-bluetooth at the very end for reference. > > > After a long git bisect, I've found the commit that's triggering the issue: > > commit 20a875e2e86e73d13ec256781a7d55a7885868ec > Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > Date: Tue Aug 23 11:33:28 2016 +0300 > > serial: 8250_dw: Add quirk for APM X-Gene SoC > > The APM X-Gene SoC UART is the only board that still needs > the hard-coded values, so handle it separately in > dw8250_quirks(). The other ACPI platforms are able to > provide the values with device properties. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > diff --git a/drivers/tty/serial/8250/8250_dw.c > b/drivers/tty/serial/8250/8250_dw.c > index e199696..5c0c123 100644 > --- a/drivers/tty/serial/8250/8250_dw.c > +++ b/drivers/tty/serial/8250/8250_dw.c > @@ -298,12 +298,17 @@ static void dw8250_quirks(struct uart_port *p, struct > dw8250_data *data) > p->serial_out = dw8250_serial_out32be; > } > } else if (has_acpi_companion(p->dev)) { > - p->iotype = UPIO_MEM32; > - p->regshift = 2; > - p->serial_in = dw8250_serial_in32; > + const struct acpi_device_id *id; > + > + id = acpi_match_device(p->dev->driver->acpi_match_table, > + p->dev); > + if (id && !strcmp(id->id, "APMC0D08")) { > + p->iotype = UPIO_MEM32; > + p->regshift = 2; > + p->serial_in = dw8250_serial_in32; > + data->uart_16550_compatible = true; > + } > p->set_termios = dw8250_set_termios; > - /* So far none of there implement the Busy Functionality */ > - data->uart_16550_compatible = true; > } > > /* Platforms with iDMA */ > > > This is fully reproducible on my end: removing this specific patch gets rid > of the issue; with the patch applied, Bluetooth simply doesn't work anymore. > > As opposed to the commit description, it seems that the Lenovo ThinkPad 8 > still needs the original hard-coded values currently (even if it could be > possible to provide them in another way in the future, if I interpret the > commit message the right way). > > Could this patch be reverted for the moment to remove the regression, until > a proper fix is found? Heikki, I don't think anything depends on this commit, is that correct? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Nov 02, 2016 at 04:49:42AM +0100, Rafael J. Wysocki wrote: > On Tue, Nov 1, 2016 at 5:58 PM, Jérôme de Bretagne > <jerome.debretagne@gmail.com> wrote: > > Hi Heikki, Andy, Greg, Rafael, > > > > I've detected a regression on my Lenovo ThinkPad 8 tablet introduced during > > 4.9-rc1, preventing the built-in Bluetooth Broadcom chipset from properly > > initializing, with many timeout messages in dmesg like these: > > "serial8250_interrupt: WXYZ callbacks suppressed" > > "serial8250: too much work for irq39" > > cf. my earlier report on linux-bluetooth at the very end for reference. Jérôme! Could you send us acpidump output and your kernel configuration. > > After a long git bisect, I've found the commit that's triggering the issue: > > > > commit 20a875e2e86e73d13ec256781a7d55a7885868ec > > Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > > Date: Tue Aug 23 11:33:28 2016 +0300 > > > > serial: 8250_dw: Add quirk for APM X-Gene SoC > > > > The APM X-Gene SoC UART is the only board that still needs > > the hard-coded values, so handle it separately in > > dw8250_quirks(). The other ACPI platforms are able to > > provide the values with device properties. > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > > Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > > > diff --git a/drivers/tty/serial/8250/8250_dw.c > > b/drivers/tty/serial/8250/8250_dw.c > > index e199696..5c0c123 100644 > > --- a/drivers/tty/serial/8250/8250_dw.c > > +++ b/drivers/tty/serial/8250/8250_dw.c > > @@ -298,12 +298,17 @@ static void dw8250_quirks(struct uart_port *p, struct > > dw8250_data *data) > > p->serial_out = dw8250_serial_out32be; > > } > > } else if (has_acpi_companion(p->dev)) { > > - p->iotype = UPIO_MEM32; > > - p->regshift = 2; > > - p->serial_in = dw8250_serial_in32; > > + const struct acpi_device_id *id; > > + > > + id = acpi_match_device(p->dev->driver->acpi_match_table, > > + p->dev); > > + if (id && !strcmp(id->id, "APMC0D08")) { > > + p->iotype = UPIO_MEM32; > > + p->regshift = 2; > > + p->serial_in = dw8250_serial_in32; > > + data->uart_16550_compatible = true; > > + } > > p->set_termios = dw8250_set_termios; > > - /* So far none of there implement the Busy Functionality */ > > - data->uart_16550_compatible = true; > > } > > > > /* Platforms with iDMA */ > > > > > > This is fully reproducible on my end: removing this specific patch gets rid > > of the issue; with the patch applied, Bluetooth simply doesn't work anymore. > > > > As opposed to the commit description, it seems that the Lenovo ThinkPad 8 > > still needs the original hard-coded values currently (even if it could be > > possible to provide them in another way in the future, if I interpret the > > commit message the right way). > > > > Could this patch be reverted for the moment to remove the regression, until > > a proper fix is found? > > Heikki, I don't think anything depends on this commit, is that correct? Unfortunately we can't fix the values for every UART that has ACPI companion like we did before anymore. There is at least one new platform, that the driver supports, which does not have the Designware UART in this "16550 compatible" mode, and I guess it's possible that there are others as well. And I guess the other values can also be what ever on those platforms. Jérôme, can you test if using the quirk on Baytrails works: @@ -302,7 +302,8 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data) id = acpi_match_device(p->dev->driver->acpi_match_table, p->dev); - if (id && !strcmp(id->id, "APMC0D08")) { + if (id && (!strcmp(id->id, "APMC0D08") || + !strcmp(id->id, "80860F0A"))) { p->iotype = UPIO_MEM32; p->regshift = 2; p->serial_in = dw8250_serial_in32; Please note that this really is not a fix, not even a temporary one for this issue. There are a lot of Baytrails on the market. I just want to make sure there really is a problem delivering those values as device properties with your board. Thanks,
Hi again, > > Heikki, I don't think anything depends on this commit, is that correct? > > Unfortunately we can't fix the values for every UART that has ACPI > companion like we did before anymore. > > There is at least one new platform, that the driver supports, which > does not have the Designware UART in this "16550 compatible" mode, and > I guess it's possible that there are others as well. And I guess the > other values can also be what ever on those platforms. > > Jérôme, can you test if using the quirk on Baytrails works: > > @@ -302,7 +302,8 @@ static void dw8250_quirks(struct uart_port *p, struct > dw8250_data *data) > > id = acpi_match_device(p->dev->driver->acpi_match_table, > p->dev); > - if (id && !strcmp(id->id, "APMC0D08")) { > + if (id && (!strcmp(id->id, "APMC0D08") || > + !strcmp(id->id, "80860F0A"))) { > p->iotype = UPIO_MEM32; > p->regshift = 2; > p->serial_in = dw8250_serial_in32; Compilation finished with that small modification applied: Bluetooth works indeed in that case and I don't have the error messages in dmesg anymore. > > Please note that this really is not a fix, not even a temporary one > for this issue. There are a lot of Baytrails on the market. I just > want to make sure there really is a problem delivering those values as > device properties with your board. I guess this confirms there is indeed a problem delivering those values as devices properties on that specific Baytrail device at least. Let me know if there is anything else useful I could share/test. Thanks, Jérôme -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 02, 2016 at 02:52:12PM +0100, Jérôme de Bretagne wrote: > Hi again, > > > > Heikki, I don't think anything depends on this commit, is that correct? > > > > Unfortunately we can't fix the values for every UART that has ACPI > > companion like we did before anymore. > > > > There is at least one new platform, that the driver supports, which > > does not have the Designware UART in this "16550 compatible" mode, and > > I guess it's possible that there are others as well. And I guess the > > other values can also be what ever on those platforms. > > > > Jérôme, can you test if using the quirk on Baytrails works: > > > > @@ -302,7 +302,8 @@ static void dw8250_quirks(struct uart_port *p, struct > > dw8250_data *data) > > > > id = acpi_match_device(p->dev->driver->acpi_match_table, > > p->dev); > > - if (id && !strcmp(id->id, "APMC0D08")) { > > + if (id && (!strcmp(id->id, "APMC0D08") || > > + !strcmp(id->id, "80860F0A"))) { > > p->iotype = UPIO_MEM32; > > p->regshift = 2; > > p->serial_in = dw8250_serial_in32; > > Compilation finished with that small modification applied: Bluetooth works > indeed in that case and I don't have the error messages in dmesg anymore. > > > > > Please note that this really is not a fix, not even a temporary one > > for this issue. There are a lot of Baytrails on the market. I just > > want to make sure there really is a problem delivering those values as > > device properties with your board. > > I guess this confirms there is indeed a problem delivering those values as > devices properties on that specific Baytrail device at least. I can reproduce this now with a Thinkpad10 we have. Cheers,
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index e199696..5c0c123 100644 --- a/drivers/tty/serial/8250/8250_dw.c +++ b/drivers/tty/serial/8250/8250_dw.c @@ -298,12 +298,17 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data) p->serial_out = dw8250_serial_out32be; } } else if (has_acpi_companion(p->dev)) { - p->iotype = UPIO_MEM32; - p->regshift = 2; - p->serial_in = dw8250_serial_in32; + const struct acpi_device_id *id; + + id = acpi_match_device(p->dev->driver->acpi_match_table, + p->dev); + if (id && !strcmp(id->id, "APMC0D08")) { + p->iotype = UPIO_MEM32; + p->regshift = 2; + p->serial_in = dw8250_serial_in32; + data->uart_16550_compatible = true; + } p->set_termios = dw8250_set_termios; - /* So far none of there implement the Busy Functionality */ - data->uart_16550_compatible = true; }