Check the 'Register Bit Width' field of the ACPI Generic Address
Structure that specifies the address of the UART registers to
decide if the driver should use "mmio32" access instead of "mmio".
If the driver is other than 16550 the access with is defined
by the Interface Type field of the SPCR table.
For discussion:
https://lkml.kernel.org/r/[email protected]
Signed-off-by: Aleksey Makarov <[email protected]>
Signed-off-by: Graeme Gregory <[email protected]>
Reported-by: Heyi Guo <[email protected]>
---
drivers/acpi/spcr.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index e8d7bc7..6c6710b 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -70,6 +70,10 @@ 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:
--
2.10.2
On Mon, Dec 5, 2016 at 5:05 AM, Aleksey Makarov
<[email protected]> wrote:
> Check the 'Register Bit Width' field of the ACPI Generic Address
> Structure that specifies the address of the UART registers to
> decide if the driver should use "mmio32" access instead of "mmio".
>
> If the driver is other than 16550 the access with is defined
> by the Interface Type field of the SPCR table.
>
> For discussion:
>
> https://lkml.kernel.org/r/[email protected]
Tested on X-Gene 1 and X-Gene 2 platforms.
Tested-by: Duc Dang <[email protected]>
>
> Signed-off-by: Aleksey Makarov <[email protected]>
> Signed-off-by: Graeme Gregory <[email protected]>
> Reported-by: Heyi Guo <[email protected]>
> ---
> drivers/acpi/spcr.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> index e8d7bc7..6c6710b 100644
> --- a/drivers/acpi/spcr.c
> +++ b/drivers/acpi/spcr.c
> @@ -70,6 +70,10 @@ 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:
> --
> 2.10.2
>
Thanks,
Duc Dang.
Duc, Aleksey, all,
I have a question about this...
On 12/05/2016 01:51 PM, Duc Dang wrote:
> On Mon, Dec 5, 2016 at 5:05 AM, Aleksey Makarov
> <[email protected]> wrote:
>> Check the 'Register Bit Width' field of the ACPI Generic Address
>> Structure that specifies the address of the UART registers to
>> decide if the driver should use "mmio32" access instead of "mmio".
>>
>> If the driver is other than 16550 the access with is defined
>> by the Interface Type field of the SPCR table.
I have two questions about this:
1). Why is this not a full 16550 (ACPI_DBG2_16550_COMPATIBLE)?
2). Why is it a ACPI_DBG2_16550_SUBSET you are assuming here?
The SPCR and DBG2 spec clearly state that the _SUBSET is intended
to represent a UART compatible with the earlier DGBP specification,
not that a UART is a "subset" of a full 16550 (which seems to be
the assumption in this patch). It's important we get this right.
I built a test kernel with this patch and updated ACPI tables earlier,
but it didn't boot with a console because I had left it a subtype 0,
but just changed the width to 32 bit, which is what I expected.
Further, I've heard back from Microsoft and they're looking at
adding a specific subtype for this. If they do, I'm inclined to
address existing designs with your patch (but I would favor this
check because against the full 16550) and then switch newer APM
based designs to the new subtype.
Jon.
--
Computer Architect | Sent from my Fedora powered laptop
Hi Jon,
On Mon, Dec 5, 2016 at 3:27 PM, Jon Masters <[email protected]> wrote:
> Duc, Aleksey, all,
>
> I have a question about this...
>
> On 12/05/2016 01:51 PM, Duc Dang wrote:
>> On Mon, Dec 5, 2016 at 5:05 AM, Aleksey Makarov
>> <[email protected]> wrote:
>>> Check the 'Register Bit Width' field of the ACPI Generic Address
>>> Structure that specifies the address of the UART registers to
>>> decide if the driver should use "mmio32" access instead of "mmio".
>>>
>>> If the driver is other than 16550 the access with is defined
>>> by the Interface Type field of the SPCR table.
>
> I have two questions about this:
>
> 1). Why is this not a full 16550 (ACPI_DBG2_16550_COMPATIBLE)?
>
> 2). Why is it a ACPI_DBG2_16550_SUBSET you are assuming here?
The patch is actually applied for both ACPI_DBG2_16550_COMPATIBLE and
ACPI_DBG2_16500_SUBSET. Or I misunderstood your question? The end
result after applying the patch on linux-next is like this:
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:
uart = "pl011";
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:
err = -ENOENT;
goto done;
}
>
> The SPCR and DBG2 spec clearly state that the _SUBSET is intended
> to represent a UART compatible with the earlier DGBP specification,
> not that a UART is a "subset" of a full 16550 (which seems to be
> the assumption in this patch). It's important we get this right.
>
> I built a test kernel with this patch and updated ACPI tables earlier,
> but it didn't boot with a console because I had left it a subtype 0,
> but just changed the width to 32 bit, which is what I expected.
On Mustang 3.06.25 firmware, DBG2 table has 'Port Type = 0x8000',
'Port subtype = 0x0001'
But I am still curious why setting subtype to '0' does not work on
your board. Are you using Mustang or m400?
>
> Further, I've heard back from Microsoft and they're looking at
> adding a specific subtype for this. If they do, I'm inclined to
> address existing designs with your patch (but I would favor this
> check because against the full 16550) and then switch newer APM
> based designs to the new subtype.
Yes, we will look out for the new subtype information.
>
> Jon.
>
> --
> Computer Architect | Sent from my Fedora powered laptop
>
Regards,
Duc Dang.
On 12/05/2016 06:52 PM, Duc Dang wrote:
> Hi Jon,
>
> On Mon, Dec 5, 2016 at 3:27 PM, Jon Masters <[email protected]> wrote:
>> Duc, Aleksey, all,
>>
>> I have a question about this...
>>
>> On 12/05/2016 01:51 PM, Duc Dang wrote:
>>> On Mon, Dec 5, 2016 at 5:05 AM, Aleksey Makarov
>>> <[email protected]> wrote:
>>>> Check the 'Register Bit Width' field of the ACPI Generic Address
>>>> Structure that specifies the address of the UART registers to
>>>> decide if the driver should use "mmio32" access instead of "mmio".
>>>>
>>>> If the driver is other than 16550 the access with is defined
>>>> by the Interface Type field of the SPCR table.
>>
>> I have two questions about this:
>>
>> 1). Why is this not a full 16550 (ACPI_DBG2_16550_COMPATIBLE)?
>>
>> 2). Why is it a ACPI_DBG2_16550_SUBSET you are assuming here?
>
> The patch is actually applied for both ACPI_DBG2_16550_COMPATIBLE and
> ACPI_DBG2_16500_SUBSET. Or I misunderstood your question?
No, I had missed the fall through for both conditions since it hadn't
worked in my quick boot test with the other type earlier. It's probably
only applicable in the general 16550 case, not in the subset case,
but I don't have any objections at this. My bad.
Now as to why it's not actually triggering on my test machine is
something I'll check. I set the port width in the address struct
in the ACPI table to 32-bit and it didn't see mmio32 just mmio, so
I then re-read the patch itself and had assumed Aleksey meant it
to be only for the subtype. Be right back after I poke...
Jon.
--
Computer Architect | Sent from my Fedora powered laptop
On 12/05/2016 06:52 PM, Duc Dang wrote:
> But I am still curious why setting subtype to '0' does not work on
> your board. Are you using Mustang or m400?
m400 with updated tables (that are correctly overriding not appending)
provided via INITRD override. I am looking at why it's not working.
[ 0.000000] ACPI: Table Upgrade: override [SPCR-HPE -ProLiant]
[ 0.000000] ACPI: SPCR 0x0000004FF7F30000 Physical table override, new table: 0x0000004FFFFF0000
[ 0.000000] ACPI: SPCR 0x0000004FFFFF0000 000050 (v02 HPE ProLiant 00001337 INTL 20160527)
Jon.
--
Computer Architect | Sent from my Fedora powered laptop
On Mon, Dec 5, 2016 at 3:52 PM, Duc Dang <[email protected]> wrote:
> Hi Jon,
>
> On Mon, Dec 5, 2016 at 3:27 PM, Jon Masters <[email protected]> wrote:
>> Duc, Aleksey, all,
>>
>> I have a question about this...
>>
>> On 12/05/2016 01:51 PM, Duc Dang wrote:
>>> On Mon, Dec 5, 2016 at 5:05 AM, Aleksey Makarov
>>> <[email protected]> wrote:
>>>> Check the 'Register Bit Width' field of the ACPI Generic Address
>>>> Structure that specifies the address of the UART registers to
>>>> decide if the driver should use "mmio32" access instead of "mmio".
>>>>
>>>> If the driver is other than 16550 the access with is defined
>>>> by the Interface Type field of the SPCR table.
>>
>> I have two questions about this:
>>
>> 1). Why is this not a full 16550 (ACPI_DBG2_16550_COMPATIBLE)?
>>
>> 2). Why is it a ACPI_DBG2_16550_SUBSET you are assuming here?
>
> The patch is actually applied for both ACPI_DBG2_16550_COMPATIBLE and
> ACPI_DBG2_16500_SUBSET. Or I misunderstood your question? The end
> result after applying the patch on linux-next is like this:
> 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:
> uart = "pl011";
> 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:
> err = -ENOENT;
> goto done;
> }
>
>>
>> The SPCR and DBG2 spec clearly state that the _SUBSET is intended
>> to represent a UART compatible with the earlier DGBP specification,
>> not that a UART is a "subset" of a full 16550 (which seems to be
>> the assumption in this patch). It's important we get this right.
>>
>> I built a test kernel with this patch and updated ACPI tables earlier,
>> but it didn't boot with a console because I had left it a subtype 0,
>> but just changed the width to 32 bit, which is what I expected.
>
> On Mustang 3.06.25 firmware, DBG2 table has 'Port Type = 0x8000',
> 'Port subtype = 0x0001'
To clarify, for SPCR table, this is what we have on Mustang:
[0004] Signature : "SPCR" [Serial Port
Console Redirection table]
[0004] Table Length : 00000050
[0001] Revision : 02
[0001] Checksum : 62
[0006] Oem ID : "APMC0D"
[0008] Oem Table ID : "XGENESPC"
[0004] Oem Revision : 00000000
[0004] Asl Compiler ID : "INTL"
[0004] Asl Compiler Revision : 20141107
[0001] Interface Type : 00
[0003] Reserved : 000000
[0012] Serial Port Register : [Generic Address Structure]
[0001] Space ID : 00 [SystemMemory]
[0001] Bit Width : 20
[0001] Bit Offset : 00
[0001] Encoded Access Width : 01 [Byte Access:8]
[0008] Address : 000000001c020000
[0001] Interrupt Type : 08
[0001] PCAT-compatible IRQ : 00
[0004] Interrupt : 0000006C
[0001] Baud Rate : 07
[0001] Parity : 00
[0001] Stop Bits : 01
[0001] Flow Control : 00
[0001] Terminal Type : 00
[0001] Reserved : 00
[0002] PCI Device ID : FFFF
[0002] PCI Vendor ID : FFFF
[0001] PCI Bus : 00
[0001] PCI Device : 00
[0001] PCI Function : 00
[0004] PCI Flags : 00000000
[0001] PCI Segment : 00
[0004] Reserved : 00000000
So the "Interface Type" is also set to 00.
>
> But I am still curious why setting subtype to '0' does not work on
> your board. Are you using Mustang or m400?
>>
>> Further, I've heard back from Microsoft and they're looking at
>> adding a specific subtype for this. If they do, I'm inclined to
>> address existing designs with your patch (but I would favor this
>> check because against the full 16550) and then switch newer APM
>> based designs to the new subtype.
>
> Yes, we will look out for the new subtype information.
>
>>
>> Jon.
>>
>> --
>> Computer Architect | Sent from my Fedora powered laptop
>>
> Regards,
> Duc Dang.
Regards,
Duc Dang.
Hi Duc, all,
So after regenerating the initrd override (I must have fat fingered)
it is now detecting the correct bit width on boot (attached dmesg log).
HOWEVER while the console does come up, the use of "earlycon" on the
command line (with no parameters) doesn't result in the early SPCR
console coming up correctly. I see some garbled characters that
suggest the baud (or register access width) is off somewhere.
Here are the first few lines from my screen boot log:
EFI stub: Using DTB from configuration table
EFI stub: Exiting boot services and installing virtual address map...
^E^B^B^B<8A>r<A2><92><A2><92>ʺ<EA><81>console [ttyS0] enabled
[ 1.424297] console [ttyS0] enabled
[ 1.507860] bootconsole [uart0] disabled
Can you double check you've actually seen the SPCR used for earlycon,
as the machine was booting, and actually generating correct output?
Here's the SPCR override I am using on that machine:
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20160527-64
* Copyright (c) 2000 - 2016 Intel Corporation
*
* Disassembly of SPCR.aml, Sat Dec 3 03:49:54 2016
*
* ACPI Data Table [SPCR]
*
* Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue
*/
[000h 0000 4] Signature : "SPCR" [Serial Port Console Redirection table]
[004h 0004 4] Table Length : 00000050
[008h 0008 1] Revision : 02
[009h 0009 1] Checksum : 41
[00Ah 0010 6] Oem ID : "HPE "
[010h 0016 8] Oem Table ID : "ProLiant"
[018h 0024 4] Oem Revision : 00001337
[01Ch 0028 4] Asl Compiler ID : "INTL"
[020h 0032 4] Asl Compiler Revision : 20160527
[024h 0036 1] Interface Type : 00
[025h 0037 3] Reserved : 000000
[028h 0040 12] Serial Port Register : [Generic Address Structure]
[028h 0040 1] Space ID : 00 [SystemMemory]
[029h 0041 1] Bit Width : 20
[02Ah 0042 1] Bit Offset : 00
[02Bh 0043 1] Encoded Access Width : 00 [Undefined/Legacy]
[02Ch 0044 8] Address : 000000001C021000
[034h 0052 1] Interrupt Type : 08
[035h 0053 1] PCAT-compatible IRQ : 00
[036h 0054 4] Interrupt : 0000006D
[03Ah 0058 1] Baud Rate : 03
[03Bh 0059 1] Parity : 00
[03Ch 0060 1] Stop Bits : 01
[03Dh 0061 1] Flow Control : 02
[03Eh 0062 1] Terminal Type : 01
[04Ch 0076 1] Reserved : 00
[040h 0064 2] PCI Device ID : FFFF
[042h 0066 2] PCI Vendor ID : FFFF
[044h 0068 1] PCI Bus : 00
[045h 0069 1] PCI Device : 00
[046h 0070 1] PCI Function : 00
[047h 0071 4] PCI Flags : 00000000
[04Bh 0075 1] PCI Segment : 00
[04Ch 0076 4] Reserved : 00000000
Raw Table Data: Length 80 (0x50)
0000: 53 50 43 52 50 00 00 00 02 41 48 50 45 4A 43 4D // SPCRP....AHPEJCM
0010: 50 72 6F 4C 69 61 6E 74 01 00 00 00 49 4E 54 4C // ProLiant....INTL
0020: 27 05 16 20 0D 00 00 00 00 08 00 00 00 10 02 1C // '.. ............
0030: 00 00 00 00 08 00 6D 00 00 00 03 00 01 02 01 00 // ......m.........
0040: FF FF FF FF 00 00 00 00 00 00 00 00 00 00 00 00 // ................
Jon.
--
Computer Architect | Sent from my Fedora powered laptop
On Mon, Dec 5, 2016 at 6:27 PM, Jon Masters <[email protected]> wrote:
> Hi Duc, all,
>
> So after regenerating the initrd override (I must have fat fingered)
> it is now detecting the correct bit width on boot (attached dmesg log).
>
> HOWEVER while the console does come up, the use of "earlycon" on the
> command line (with no parameters) doesn't result in the early SPCR
> console coming up correctly. I see some garbled characters that
> suggest the baud (or register access width) is off somewhere.
My bad that I did not catch this in the morning. Yes, earlycon does
not seems to work as expected. I can see that earlycon parameters
seems to be correct, but the bootconsole message does not come out
(the following is from 'dmesg')
[ 0.000000] Booting Linux on physical CPU 0x0
[ 0.000000] Linux version 4.9.0-rc7-next-20161202-00001-gbf2919a
(dhdang@dhdang-workstation-01) (gcc version 4.9.3 20150218
(prerelease) (APM-8.0.10-le) ) #10 SMP PREEMPT Mon Dec 5 19:21:14 PST
2016
[ 0.000000] Boot CPU: AArch64 Processor [500f0001]
[ 0.000000] efi: Getting EFI parameters from FDT:
[ 0.000000] efi: EFI v2.40 by X-Gene Mustang Board EFI Oct 17 2016 13:54:05
[ 0.000000] efi: ACPI=0x47fa700000 ACPI 2.0=0x47fa700014 SMBIOS
3.0=0x47fa9db000 ESRT=0x47ff006d18
[ 0.000000] esrt: Reserving ESRT space from 0x00000047ff006d18 to
0x00000047ff006d78.
[ 0.000000] cma: Reserved 16 MiB at 0x00000040ff000000
[ 0.000000] ACPI: Early table checksum verification disabled
[ 0.000000] ACPI: RSDP 0x00000047FA700014 000024 (v02 APM )
[ 0.000000] ACPI: XSDT 0x00000047FA6F00E8 000084 (v01 APM XGENE
00000003 01000013)
[ 0.000000] ACPI: FACP 0x00000047FA6C0000 00010C (v05 APM XGENE
00000003 INTL 20140724)
[ 0.000000] ACPI: DSDT 0x00000047FA6D0000 005922 (v05 APM
APM88xxx 00000001 INTL 20140724)
[ 0.000000] ACPI: DBG2 0x00000047FA6E0000 0000AA (v00 APMC0D
XGENEDBG 00000000 INTL 20140724)
[ 0.000000] ACPI: GTDT 0x00000047FA6A0000 000060 (v02 APM XGENE
00000001 INTL 20140724)
[ 0.000000] ACPI: MCFG 0x00000047FA690000 00003C (v01 APM XGENE
00000002 INTL 20140724)
[ 0.000000] ACPI: SPCR 0x00000047FA680000 000050 (v02 APMC0D
XGENESPC 00000000 INTL 20140724)
[ 0.000000] ACPI: SSDT 0x00000047FA670000 00002D (v02 APM XGENE
00000001 INTL 20140724)
[ 0.000000] ACPI: BERT 0x00000047FA660000 000030 (v01 APM XGENE
00000002 INTL 20140724)
[ 0.000000] ACPI: HEST 0x00000047FA650000 0002A8 (v01 APM XGENE
00000002 INTL 20140724)
[ 0.000000] ACPI: APIC 0x00000047FA640000 0002A4 (v03 APM XGENE
00000003 01000013)
[ 0.000000] ACPI: SSDT 0x00000047FA630000 000063 (v02 REDHAT
MACADDRS 00000001 01000013)
[ 0.000000] ACPI: SSDT 0x00000047FA620000 000032 (v02 REDHAT
UARTCLKS 00000001 01000013)
[ 0.000000] ACPI: PCCT 0x00000047FA610000 000300 (v01 APM XGENE
00000003 01000013)
[ 0.000000] ACPI: SPCR: console: uart,mmio32,0x1c020000,115200
[ 0.000000] earlycon: uart0 at MMIO32 0x000000001c020000 (options '115200')
[ 0.000000] bootconsole [uart0] enabled
>
> Here are the first few lines from my screen boot log:
>
> EFI stub: Using DTB from configuration table
> EFI stub: Exiting boot services and installing virtual address map...
> ^E^B^B^B<8A>r<A2><92><A2><92>ʺ<EA><81>console [ttyS0] enabled
I do not see garbage characters on Mustang. The garbage characters you
saw may relate to the same issue that I asked a few weeks ago:
https://www.spinics.net/lists/arm-kernel/msg537958.html
There seems to be a race condition in accessing the UART hardware
during switching between the bootconsole and the real console.
> [ 1.424297] console [ttyS0] enabled
> [ 1.507860] bootconsole [uart0] disabled
>
> Can you double check you've actually seen the SPCR used for earlycon,
> as the machine was booting, and actually generating correct output?
>
> Here's the SPCR override I am using on that machine:
>
> /*
> * Intel ACPI Component Architecture
> * AML/ASL+ Disassembler version 20160527-64
> * Copyright (c) 2000 - 2016 Intel Corporation
> *
> * Disassembly of SPCR.aml, Sat Dec 3 03:49:54 2016
> *
> * ACPI Data Table [SPCR]
> *
> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue
> */
>
> [000h 0000 4] Signature : "SPCR" [Serial Port Console Redirection table]
> [004h 0004 4] Table Length : 00000050
> [008h 0008 1] Revision : 02
> [009h 0009 1] Checksum : 41
> [00Ah 0010 6] Oem ID : "HPE "
> [010h 0016 8] Oem Table ID : "ProLiant"
> [018h 0024 4] Oem Revision : 00001337
> [01Ch 0028 4] Asl Compiler ID : "INTL"
> [020h 0032 4] Asl Compiler Revision : 20160527
>
> [024h 0036 1] Interface Type : 00
> [025h 0037 3] Reserved : 000000
>
> [028h 0040 12] Serial Port Register : [Generic Address Structure]
> [028h 0040 1] Space ID : 00 [SystemMemory]
> [029h 0041 1] Bit Width : 20
> [02Ah 0042 1] Bit Offset : 00
> [02Bh 0043 1] Encoded Access Width : 00 [Undefined/Legacy]
> [02Ch 0044 8] Address : 000000001C021000
>
> [034h 0052 1] Interrupt Type : 08
> [035h 0053 1] PCAT-compatible IRQ : 00
> [036h 0054 4] Interrupt : 0000006D
> [03Ah 0058 1] Baud Rate : 03
> [03Bh 0059 1] Parity : 00
> [03Ch 0060 1] Stop Bits : 01
> [03Dh 0061 1] Flow Control : 02
> [03Eh 0062 1] Terminal Type : 01
> [04Ch 0076 1] Reserved : 00
> [040h 0064 2] PCI Device ID : FFFF
> [042h 0066 2] PCI Vendor ID : FFFF
> [044h 0068 1] PCI Bus : 00
> [045h 0069 1] PCI Device : 00
> [046h 0070 1] PCI Function : 00
> [047h 0071 4] PCI Flags : 00000000
> [04Bh 0075 1] PCI Segment : 00
> [04Ch 0076 4] Reserved : 00000000
>
> Raw Table Data: Length 80 (0x50)
>
> 0000: 53 50 43 52 50 00 00 00 02 41 48 50 45 4A 43 4D // SPCRP....AHPEJCM
> 0010: 50 72 6F 4C 69 61 6E 74 01 00 00 00 49 4E 54 4C // ProLiant....INTL
> 0020: 27 05 16 20 0D 00 00 00 00 08 00 00 00 10 02 1C // '.. ............
> 0030: 00 00 00 00 08 00 6D 00 00 00 03 00 01 02 01 00 // ......m.........
> 0040: FF FF FF FF 00 00 00 00 00 00 00 00 00 00 00 00 // ................
>
> Jon.
>
> --
> Computer Architect | Sent from my Fedora powered laptop
>
Regards,
Duc Dang.
On 12/05/2016 10:55 PM, Duc Dang wrote:
> On Mon, Dec 5, 2016 at 6:27 PM, Jon Masters <[email protected]> wrote:
>> Hi Duc, all,
>>
>> So after regenerating the initrd override (I must have fat fingered)
>> it is now detecting the correct bit width on boot (attached dmesg log).
>>
>> HOWEVER while the console does come up, the use of "earlycon" on the
>> command line (with no parameters) doesn't result in the early SPCR
>> console coming up correctly. I see some garbled characters that
>> suggest the baud (or register access width) is off somewhere.
>
> My bad that I did not catch this in the morning. Yes, earlycon does
> not seems to work as expected. I can see that earlycon parameters
> seems to be correct, but the bootconsole message does not come out
> (the following is from 'dmesg')
> [ 0.000000] ACPI: SPCR: console: uart,mmio32,0x1c020000,115200
> [ 0.000000] earlycon: uart0 at MMIO32 0x000000001c020000 (options '115200')
> [ 0.000000] bootconsole [uart0] enabled
The difference appears to be in the baud rate. When I explicitly specify
"earlycon=uart8250,mmio32,0x1c021000" no baud is set. When booting with
the SPCR, the baud is set to 9600 in my case or 115200 in yours. But we
both know that the base clock on the X-Gene UART is weird. Maybe
somehow we can explain this through the lack of setting a baud.
I am pondering some more currently. If it's X-Gene specific, let's add
that to the quirk (and to perhaps a more APM specific SPCR subtype).
Jon.
--
Computer Architect | Sent from my Fedora powered laptop
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 <[email protected]> wrote:
>>> Hi Duc, all,
>>>
>>> So after regenerating the initrd override (I must have fat fingered)
>>> it is now detecting the correct bit width on boot (attached dmesg log).
>>>
>>> HOWEVER while the console does come up, the use of "earlycon" on the
>>> command line (with no parameters) doesn't result in the early SPCR
>>> console coming up correctly. I see some garbled characters that
>>> suggest the baud (or register access width) is off somewhere.
>>
>> My bad that I did not catch this in the morning. Yes, earlycon does
>> not seems to work as expected. I can see that earlycon parameters
>> seems to be correct, but the bootconsole message does not come out
>> (the following is from 'dmesg')
>
>> [ 0.000000] ACPI: SPCR: console: uart,mmio32,0x1c020000,115200
>> [ 0.000000] earlycon: uart0 at MMIO32 0x000000001c020000 (options '115200')
>> [ 0.000000] bootconsole [uart0] enabled
>
> The difference appears to be in the baud rate. When I explicitly specify
> "earlycon=uart8250,mmio32,0x1c021000" no baud is set. When booting with
> the SPCR, the baud is set to 9600 in my case or 115200 in yours. But we
> both know that the base clock on the X-Gene UART is weird. Maybe
> somehow we can explain this through the lack of setting a baud.
>
> I am pondering some more currently. If it's X-Gene specific, let's add
> that to the quirk (and to perhaps a more APM specific SPCR subtype).
Yeah, that's it. Here's the logic (8250_early.c):
if (!device->baud) {
struct uart_port *port = &device->port;
unsigned int ier;
/* assume the device was initialized, only mask interrupts */
ier = serial8250_early_in(port, UART_IER);
serial8250_early_out(port, UART_IER, ier & UART_IER_UUE);
} else
init_port(device);
If we have a baud set we will call init_port and go messing with the
8250 UART clock and so on. While if we don't have one set we'll assume
whatever the firmware gave to us. We know the base clock frequency is
different, which made me wonder how the full 8250_dw drive was working
on your hardware...until I noticed the following ;)
Here's a snippet of the DSDT on m400:
Device (UAR0)
{
Name (_HID, "APMC0D08") // _HID: Hardware ID
Name (_DDN, "UAR0") // _DDN: DOS Device Name
Name (_UID, "UAR0") // _UID: Unique ID
Name (_STR, Unicode ("APM88xxxx UART0 Controller")) // _STR: Descri
ption String
Name (_ADR, 0x1C021000) // _ADR: Address
Name (_CID, "NS16550A") // _CID: Compatible ID
...
Method (_DSD, 0, NotSerialized) // _DSD: Device-Specific Data
{
Local0 = Package (0x02)
{
ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
Package (0x01)
{
Package (0x02)
{
"clock-frequency",
Zero
}
}
}
DerefOf (DerefOf (Local0 [One]) [Zero]) [One]
= UCLK /* External reference */
Return (Local0)
}
...and here's what's sneakily in the first SSDT table:
DefinitionBlock ("", "SSDT", 2, "HPE ", "UARTCLKS", 0x00000001)
{
Name (\UCLK, Package (0x01)
{
0x02FAF080
})
}
I suspect Mustang is doing similar. So you read the magic frequency
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.
We'll need to accept that on X-Gene platforms earlycon doesn't quite
work properly for the moment until we can add the correct quirk. So
Aleksey's patch kinda improves things in that it brings up the
console (which didn't work before) and therefore is hugely
beneficial, but it won't be able to enable the earlycon.
Aleksey: you might want to annotate your patch thusly:
"Discussion is still underway to add a new DBG2 (the table used to
enumerate the various subtypes of serial port covered by the SPCR)
16550 UART subtype that may be needed for some additional platforms,
such as those based upon AppliedMicro X-Gene ARMv8 SoCs"
Your patch as-is /is/ useful because it enables the console, and
most users care far more about that than the earlycon.
Jon.
P.S. I would note my usual anal pedantry. The reason I'm being a
pedant here is that we need to test this stuff as a user would use
it - making sure we get console output at the right moment - rather
than just boot testing and checking logs. That's never the same.
--
Computer Architect | Sent from my Fedora powered laptop
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 <[email protected]> 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).
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.
--
Computer Architect | Sent from my Fedora powered laptop
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 <[email protected]> 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:
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);
------------------------------
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.
>
On Tue, 2016-12-06 at 01:34 -0500, Jon Masters wrote:
> On 12/05/2016 10:55 PM, Duc Dang wrote:
> >
> > On Mon, Dec 5, 2016 at 6:27 PM, Jon Masters <[email protected]> wrote:
> > >
> > > Hi Duc, all,
> > >
> > > So after regenerating the initrd override (I must have fat fingered)
> > > it is now detecting the correct bit width on boot (attached dmesg log).
> > >
> > > HOWEVER while the console does come up, the use of "earlycon" on the
> > > command line (with no parameters) doesn't result in the early SPCR
> > > console coming up correctly. I see some garbled characters that
> > > suggest the baud (or register access width) is off somewhere.
> > My bad that I did not catch this in the morning. Yes, earlycon does
> > not seems to work as expected. I can see that earlycon parameters
> > seems to be correct, but the bootconsole message does not come out
> > (the following is from 'dmesg')
> >
> > [ 0.000000] ACPI: SPCR: console: uart,mmio32,0x1c020000,115200
> > [ 0.000000] earlycon: uart0 at MMIO32 0x000000001c020000 (options '115200')
> > [ 0.000000] bootconsole [uart0] enabled
> The difference appears to be in the baud rate. When I explicitly specify
> "earlycon=uart8250,mmio32,0x1c021000" no baud is set. When booting with
> the SPCR, the baud is set to 9600 in my case or 115200 in yours. But we
> both know that the base clock on the X-Gene UART is weird. Maybe
> somehow we can explain this through the lack of setting a baud.
BTW, this behavior is same with devicetree.
If you specify a baudrate with earlycon=, the driver tries to set that
baudrate and if you have an 8250 with some non-standard baud clock, then
it will fail. Perhaps SPCR shouldn't pass baud option to setup_earlycon().
Then again, setup_earlycon() has this comment:
* setup_earlycon - match and register earlycon console
* @buf: earlycon param string
*
* Registers the earlycon console matching the earlycon specified
* in the param string @buf. Acceptable param strings are of the form
* <name>,io|mmio|mmio32|mmio32be,<addr>,<options>
* <name>,0x<addr>,<options>
* <name>,<options>
* <name>
*
* Only for the third form does the earlycon setup() method receive the
* <options> string in the 'options' parameter; all other forms set
* the parameter to NULL.
That part about the 3rd form doesn't seem to be true. I don't see where
options gets set to NULL for forms other than the third.
>
> I am pondering some more currently. If it's X-Gene specific, let's add
> that to the quirk (and to perhaps a more APM specific SPCR subtype).
>
> Jon.
>
On 12/07/2016 10:23 AM, Mark Salter wrote:
> On Tue, 2016-12-06 at 01:34 -0500, Jon Masters wrote:
>> On 12/05/2016 10:55 PM, Duc Dang wrote:
>>> On Mon, Dec 5, 2016 at 6:27 PM, Jon Masters <[email protected]> wrote:
>>>> HOWEVER while the console does come up, the use of "earlycon" on the
>>>> command line (with no parameters) doesn't result in the early SPCR
>>>> console coming up correctly. I see some garbled characters that
>>>> suggest the baud (or register access width) is off somewhere.
>>> My bad that I did not catch this in the morning. Yes, earlycon does
>>> not seems to work as expected. I can see that earlycon parameters
>>> seems to be correct, but the bootconsole message does not come out
>>> (the following is from 'dmesg')
>>>
>>> [ 0.000000] ACPI: SPCR: console: uart,mmio32,0x1c020000,115200
>>> [ 0.000000] earlycon: uart0 at MMIO32 0x000000001c020000 (options '115200')
>>> [ 0.000000] bootconsole [uart0] enabled
>> The difference appears to be in the baud rate. When I explicitly specify
>> "earlycon=uart8250,mmio32,0x1c021000" no baud is set. When booting with
>> the SPCR, the baud is set to 9600 in my case or 115200 in yours. But we
>> both know that the base clock on the X-Gene UART is weird. Maybe
>> somehow we can explain this through the lack of setting a baud.
>
> BTW, this behavior is same with devicetree.
At least it's consistent :)
> If you specify a baudrate with earlycon=, the driver tries to set that
> baudrate and if you have an 8250 with some non-standard baud clock, then
> it will fail. Perhaps SPCR shouldn't pass baud option to setup_earlycon().
Yet they seem to explicitly want to do this...in my conversations with some
others we agree that, in many cases, you really want to say "leave the baud
whatever the firmware set it", which would work in this case, but might
break some others. Then again, nobody on x86 Linux is really using the
SPCR today due to it not having been something they used until now and
due to the location of the COM ports being fairly well known ;)
So who knows what folks will prefer, but we should at least get the spec
to cover both situations by explicitly calling out Applied as special.
> Then again, setup_earlycon() has this comment:
>
> * setup_earlycon - match and register earlycon console
> * @buf: earlycon param string
> *
> * Registers the earlycon console matching the earlycon specified
> * in the param string @buf. Acceptable param strings are of the form
> * <name>,io|mmio|mmio32|mmio32be,<addr>,<options>
> * <name>,0x<addr>,<options>
> * <name>,<options>
> * <name>
> *
> * Only for the third form does the earlycon setup() method receive the
> * <options> string in the 'options' parameter; all other forms set
> * the parameter to NULL.
>
> That part about the 3rd form doesn't seem to be true. I don't see where
> options gets set to NULL for forms other than the third.
I saw that also, and agree that it appears totally bogus. We'll want to get
the documentation fixed as part of this cleanup.
So I've been discussing some changes to the SPCR and the current proposal
is that we have two new subtypes - one for 16550s that are non-standard
register width/stride but use the typical base clock, and a specific
additional type for SBSA level 0 compatible 16550 UARTs (Applied). I
will followup when the specification document has been revised.
Jon.
P.S. I had a lot of fun over my birthday reading the original 8250 spec
and learning about how the DLAB stuff works (I think those brain cells had
died). Which - on a total tangent - finally helps with a long standing issue
we have had. We (a small cabal) have wanted to sneak in an instruction into
the ARM ISA or specs referring to DLAB (the initials of a certain person
who owns the ARM ARM) and now we will get to do this into the SBBR ;)
--
Computer Architect | Sent from my Fedora powered laptop
On 12/13/2016 01:20 AM, Jon Masters wrote:
> On 12/07/2016 10:23 AM, Mark Salter wrote:
>> If you specify a baudrate with earlycon=, the driver tries to set that
>> baudrate and if you have an 8250 with some non-standard baud clock, then
>> it will fail. Perhaps SPCR shouldn't pass baud option to setup_earlycon().
>
> Yet they seem to explicitly want to do this...in my conversations with some
> others we agree that, in many cases, you really want to say "leave the baud
> whatever the firmware set it", which would work in this case, but might
> break some others. Then again, nobody on x86 Linux is really using the
> SPCR today due to it not having been something they used until now and
> due to the location of the COM ports being fairly well known ;)
>
> So who knows what folks will prefer, but we should at least get the spec
> to cover both situations by explicitly calling out Applied as special.
<snip>
> So I've been discussing some changes to the SPCR and the current proposal
> is that we have two new subtypes - one for 16550s that are non-standard
> register width/stride but use the typical base clock, and a specific
> additional type for SBSA level 0 compatible 16550 UARTs (Applied). I
> will followup when the specification document has been revised.
As an update. I've been speaking with friends at Microsoft on and off
about this for quite some time. They've been very helpful (as usual) and
we should get an SPCR update soon covering Applied's case. I have pinged
the APM team to make sure that they're ready to post a patch.
I would like this small fix squeezed in 4.12, but before 4.13.
Jon.