diff mbox

SPCR: check bit width for the 16550 UART

Message ID d114928b-172c-0140-9a97-4550bd5bbf5d@linaro.org
State New
Headers show

Commit Message

Aleksey Makarov Dec. 6, 2016, 8:40 a.m. UTC 
Hi Jon,

On 12/06/2016 01:13 PM, Jon Masters wrote:
> On 12/06/2016 01:53 AM, Jon Masters wrote:

>> On 12/06/2016 01:34 AM, Jon Masters wrote:

>>> On 12/05/2016 10:55 PM, Duc Dang wrote:

>>>> On Mon, Dec 5, 2016 at 6:27 PM, Jon Masters <jcm@redhat.com> wrote:

>

>>                         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,

>>                         Package (0x01)

>>                         {

>>                             Package (0x02)

>>                             {

>>                                 "clock-frequency",

>>                                 Zero

>>                             }

>>                         }

>

>> I suspect Mustang is doing similar. So you read the magic frequency

>                        ^^^^^^^^^^^^^

>                      spoiler, it is :)

>

>> info and pass this in through a DSD in the AML. It's ok, I've made my

>> peace with this (but obviously we're trying to kill all DSD hacks),

>> but it explains how this "works" after boot, but not for early console.

>>

>> To fix this, we're going to need to be able to know that your 8250 is

>> both needful of 32-bit accesses *AND* needs a different base UART clock.

>> Fortunately our good friends at Microsoft are amenable to adding a

>> subtype that covers this and are going to followup tomorrow for me.

>

> For the earlycon setup, you'll probably benefit from figuring out

> ahead of time how you'll want to handle this. The "easy" option

> is to remove the baud in the case of X-Gene with new subtype

> but that'll be potential fragile. You might also decide that

> struct earlycon_device wants to grow a "quirks" flag that

> you can set for this kind of thing (bound to be more).


I don't see why new subtypes or quirk flags are required.

My suggestion is to change SPCR spec so that

- Access width should be specified by the "Bit width" subfield of the
   "Base address" field.
   When "Interface type" is ACPI_DBG2_ARM_SBSA_32BIT, disregard this value
   and use mmio32 (legacy case)

- Add a new value 0 (now it is reserved) for "Baud rate" field
   with meaning "do not change pre-initialized clock rate"

Then spcr.c should be fixed this way:

------------------------------

All the best
Aleksey Makarov

> Copying Rob as well in case he has suggestions for you. Rob, as

> an FYI the AppliedMicro X-Gene 16550 "compatible" requires both

> 32-bit accesses (which Aleksey tried to address earlier in this

> thread) but also has a special non-standard clock (to be fair,

> it's not as if we architected a standard UART clock for arm64,

> we just assumed it was like x86 but didn't mandate this early

> when Applied were doing their design - another life lesson).

> Microsoft will add a new DBG2 subtype that allows for both

> of these situations. But you'll want to somehow convey this

> information (quirks) in one of the earlycon structures.

>

> Jon.

>

--
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
diff mbox

Patch

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 6c6710b..9c6c0b4 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -37,7 +37,7 @@  int __init parse_spcr(bool earlycon)
  	acpi_status status;
  	char *uart;
  	char *iotype;
-	int baud_rate;
+	char *baud_rate_string;
  	int err;
  
  	if (acpi_disabled)
@@ -56,13 +56,19 @@  int __init parse_spcr(bool earlycon)
  		goto done;
  	}
  
-	iotype = table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY ?
-			"mmio" : "io";
+	if (table->interface_type == ACPI_DBG2_ARM_SBSA_32BIT) {
+		iotype = "mmio32";
+	} else if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+		if (table->serial_port.bit_width == 32)
+			iotype = "mmio32";
+		else
+			iotype = "mmio";
+	} else {
+		iotype = "io";
+	}
  
  	switch (table->interface_type) {
  	case ACPI_DBG2_ARM_SBSA_32BIT:
-		iotype = "mmio32";
-		/* fall through */
  	case ACPI_DBG2_ARM_PL011:
  	case ACPI_DBG2_ARM_SBSA_GENERIC:
  	case ACPI_DBG2_BCM2835:
@@ -70,10 +76,6 @@  int __init parse_spcr(bool earlycon)
  		break;
  	case ACPI_DBG2_16550_COMPATIBLE:
  	case ACPI_DBG2_16550_SUBSET:
-		if (table->serial_port.space_id ==
-			ACPI_ADR_SPACE_SYSTEM_MEMORY &&
-		    table->serial_port.bit_width == 32)
-			iotype = "mmio32";
  		uart = "uart";
  		break;
  	default:
@@ -82,25 +84,33 @@  int __init parse_spcr(bool earlycon)
  	}
  
  	switch (table->baud_rate) {
+	case 0:
+		/*
+		 * This value is not standaritzed by ACPI SPCR for now.
+		 * It means that hardware should not be re-initialized with
+		 * new speed.
+		 */
+		baud_rate_string = "";
+		break;
  	case 3:
-		baud_rate = 9600;
+		baud_rate_string = ",9600";
  		break;
  	case 4:
-		baud_rate = 19200;
+		baud_rate_string = ",19200";
  		break;
  	case 6:
-		baud_rate = 57600;
+		baud_rate_string = ",57600";
  		break;
  	case 7:
-		baud_rate = 115200;
+		baud_rate_string = ",115200";
  		break;
  	default:
  		err = -ENOENT;
  		goto done;
  	}
  
-	snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
-		 table->serial_port.address, baud_rate);
+	snprintf(opts, sizeof(opts), "%s,%s,0x%llx%s", uart, iotype,
+		 table->serial_port.address, baud_rate_string);
  
  	pr_info("console: %s\n", opts);