| Message ID | 20161205130534.11080-1-aleksey.makarov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Dec 5, 2016 at 5:05 AM, Aleksey Makarov <aleksey.makarov@linaro.org> 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/7fa523de-3fbb-1566-f521-927143f73d1e@redhat.com Tested on X-Gene 1 and X-Gene 2 platforms. Tested-by: Duc Dang <dhdang@apm.com> > > Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org> > Reported-by: Heyi Guo <heyi.guo@linaro.org> > --- > 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. -- 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
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 > <aleksey.makarov@linaro.org> 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.
Hi Jon, On Mon, Dec 5, 2016 at 3:27 PM, Jon Masters <jcm@redhat.com> 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 >> <aleksey.makarov@linaro.org> 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. -- 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 12/05/2016 06:52 PM, Duc Dang wrote: > Hi Jon, > > On Mon, Dec 5, 2016 at 3:27 PM, Jon Masters <jcm@redhat.com> 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 >>> <aleksey.makarov@linaro.org> 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.
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.
On Mon, Dec 5, 2016 at 3:52 PM, Duc Dang <dhdang@apm.com> wrote: > Hi Jon, > > On Mon, Dec 5, 2016 at 3:27 PM, Jon Masters <jcm@redhat.com> 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 >>> <aleksey.makarov@linaro.org> 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. -- 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 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.
On Mon, Dec 5, 2016 at 6:27 PM, Jon Masters <jcm@redhat.com> 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. -- 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 12/05/2016 10:55 PM, Duc Dang wrote: > On Mon, Dec 5, 2016 at 6:27 PM, Jon Masters <jcm@redhat.com> 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.
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: >>> 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.
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). 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 <jcm@redhat.com> 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. > -- 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 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 <jcm@redhat.com> 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 ;)
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. -- 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 --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: