| Message ID | 20181115175854.7550-6-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | ACPI: Generate SPCR table | expand |
On 15.11.18 18:58, Andy Shevchenko wrote: > Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1]. > Let's provide it in U-Boot. > > [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > arch/x86/include/asm/acpi_table.h | 2 + > arch/x86/lib/acpi_table.c | 85 +++++++++++++++++++++++++++++++ > 2 files changed, 87 insertions(+) > > diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h > index 51cc806673..e3b65cff66 100644 > --- a/arch/x86/include/asm/acpi_table.h > +++ b/arch/x86/include/asm/acpi_table.h > @@ -327,6 +327,8 @@ struct acpi_global_nvs; > #define ACPI_DBG2_USB_XHCI 0x0000 > #define ACPI_DBG2_USB_EHCI 0x0001 > > +#define ACPI_DBG2_UNKNOWN 0x00FF > + > /* SPCR (Serial Port Console Redirection table) */ > struct __packed acpi_spcr { > struct acpi_table_header header; > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c > index c6b2026613..d8b44aea02 100644 > --- a/arch/x86/lib/acpi_table.c > +++ b/arch/x86/lib/acpi_table.c > @@ -12,6 +12,7 @@ > #include <cpu.h> > #include <dm.h> > #include <dm/uclass-internal.h> > +#include <serial.h> > #include <version.h> > #include <asm/acpi/global_nvs.h> > #include <asm/acpi_table.h> > @@ -338,6 +339,82 @@ static void acpi_create_mcfg(struct acpi_mcfg *mcfg) > header->checksum = table_compute_checksum((void *)mcfg, header->length); > } > > +static void acpi_create_spcr(struct acpi_spcr *spcr) > +{ > + struct acpi_table_header *header = &(spcr->header); > + struct serial_device_info info = {0}; > + int access_size; > + int ret; > + > + /* Fill out header fields */ > + acpi_fill_header(header, "SPCR"); > + header->length = sizeof(struct acpi_spcr); > + header->revision = 2; > + > + ret = serial_getinfo(&info); > + if (ret) > + spcr->interface_type = ACPI_DBG2_UNKNOWN; > + else > + spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE; This sounds like pretty subtle semantics. Could you just include the interface type in &info? The main problem I'm seeing is that your UART might be a PL011 compatible which could still implement getinfo() but then wouldn't be 16660 compatible. > + debug("UART type %u (serial_getinfo() rc=%d)\n", spcr->interface_type, ret); > + > + /* Encode baud rate */ > + switch (info.baudrate) { > + case 9600: > + spcr->baud_rate = 3; > + break; > + case 19200: > + spcr->baud_rate = 4; > + break; > + case 57600: > + spcr->baud_rate = 6; > + break; > + case 115200: > + spcr->baud_rate = 7; > + break; Are any higher (and lower) ones specified too? If so, you may want to add them as well. > + default: > + spcr->baud_rate = 0; > + break; > + } > + > + /* Encode register access size */ > + switch (info.reg_shift) { > + case 0: > + access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS; > + break; > + case 1: > + access_size = ACPI_ACCESS_SIZE_WORD_ACCESS; > + break; > + case 2: > + access_size = ACPI_ACCESS_SIZE_DWORD_ACCESS; > + break; > + case 3: > + access_size = ACPI_ACCESS_SIZE_QWORD_ACCESS; > + break; > + default: > + access_size = ACPI_ACCESS_SIZE_UNDEFINED; > + break; > + } > + > + spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY; Can you guarantee that? Should probably be part of &info as well if addr is. > + spcr->serial_port.bit_width = info.reg_width; > + spcr->serial_port.bit_offset = info.reg_offset; > + spcr->serial_port.access_size = access_size; > + spcr->serial_port.addrl = info.addr >> 0; > + spcr->serial_port.addrh = info.addr >> 32; > + > + /* REVISIT: Hard coded values for now */ > + spcr->parity = 0; > + spcr->stop_bits = 1; IMHO those should be part of &info as well. If you have to hard code them, better hard code them in the driver rather than the generic ACPI generation code. Alex > + > + /* REVISIT: No PCI devices for now */ > + spcr->pci_device_id = 0xffff; > + spcr->pci_vendor_id = 0xffff; > + > + /* Fix checksum */ > + header->checksum = table_compute_checksum((void *)spcr, header->length); > +} > + > /* > * QEMU's version of write_acpi_tables is defined in drivers/misc/qfw.c > */ > @@ -352,6 +429,7 @@ ulong write_acpi_tables(ulong start) > struct acpi_fadt *fadt; > struct acpi_mcfg *mcfg; > struct acpi_madt *madt; > + struct acpi_spcr *spcr; > int i; > > current = start; > @@ -440,6 +518,13 @@ ulong write_acpi_tables(ulong start) > acpi_add_table(rsdp, mcfg); > current = ALIGN(current, 16); > > + debug("ACPI: * SPCR\n"); > + spcr = (struct acpi_spcr *)current; > + acpi_create_spcr(spcr); > + current += spcr->header.length; > + acpi_add_table(rsdp, spcr); > + current = ALIGN(current, 16); > + > debug("current = %x\n", current); > > acpi_rsdp_addr = (unsigned long)rsdp; >
On Thu, Nov 15, 2018 at 8:27 PM Alexander Graf <agraf@suse.de> wrote: > On 15.11.18 18:58, Andy Shevchenko wrote: > > Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1]. > > Let's provide it in U-Boot. > > > > [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx > > + ret = serial_getinfo(&info); > > + if (ret) > > + spcr->interface_type = ACPI_DBG2_UNKNOWN; > > + else > > + spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE; > > This sounds like pretty subtle semantics. Could you just include the > interface type in &info? It might be another step if you provide a cool way to do so. Otherwise we need first to de-x86 acpi stuff. I won't do it right now since it so-o out of the scope of this series. > The main problem I'm seeing is that your UART might be a PL011 > compatible which could still implement getinfo() but then wouldn't be > 16660 compatible. Yes, I know. Perhaps I forgot to mark it in "Known issues" section. Have you seen, btw, PL011 on x86 environment? > Are any higher (and lower) ones specified too? If so, you may want to > add them as well. There is a link to a specification. Care to read? > > + default: > > + spcr->baud_rate = 0; > > + break; Going ahead. This part actually something to develop through specifications, but I need to ping Microsoft and our internal ACPI people for that. > > + break; > > + default: > > + access_size = ACPI_ACCESS_SIZE_UNDEFINED; > > + break; > > + } > > + > > + spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY; > > Can you guarantee that? Should probably be part of &info as well if addr is. Not now. As per moving to info, see my comment about de-x86 of ACPI in U-Boot. > > + /* REVISIT: Hard coded values for now */ > > + spcr->parity = 0; > > + spcr->stop_bits = 1; > > IMHO those should be part of &info as well. If you have to hard code > them, better hard code them in the driver rather than the generic ACPI > generation code. Care to read cover letter carefully?
On 15.11.18 20:43, Andy Shevchenko wrote: > On Thu, Nov 15, 2018 at 8:27 PM Alexander Graf <agraf@suse.de> wrote: >> On 15.11.18 18:58, Andy Shevchenko wrote: >>> Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1]. >>> Let's provide it in U-Boot. >>> >>> [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx > >>> + ret = serial_getinfo(&info); >>> + if (ret) >>> + spcr->interface_type = ACPI_DBG2_UNKNOWN; >>> + else >>> + spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE; >> >> This sounds like pretty subtle semantics. Could you just include the >> interface type in &info? > > It might be another step if you provide a cool way to do so. Otherwise Define an enum with serial_type where 0 is unknown and 1 is 16550_compatible. Provide that enum in getinfo(). Bonus points for keeping the enum and ACPI_DBG2 IDs in sync, so that we don't need too much code to copy from one to the other. > we need first to de-x86 acpi stuff. I won't do it right now since it > so-o out of the scope of this series. Well, we will probably want to be able to do that in the future. And anything that hard codes more x86 assumptions is a step in the wrong direction. > >> The main problem I'm seeing is that your UART might be a PL011 >> compatible which could still implement getinfo() but then wouldn't be >> 16660 compatible. > > Yes, I know. Perhaps I forgot to mark it in "Known issues" section. > > Have you seen, btw, PL011 on x86 environment? I can create one for you in QEMU if you like? > >> Are any higher (and lower) ones specified too? If so, you may want to >> add them as well. > > There is a link to a specification. Care to read? A simple "no, the spec only defines the ones above" would've been a better answer and saved both of us a few seconds of our lives ;). > >>> + default: >>> + spcr->baud_rate = 0; >>> + break; > > Going ahead. > This part actually something to develop through specifications, but I > need to ping Microsoft and our internal ACPI people for that. > >>> + break; >>> + default: >>> + access_size = ACPI_ACCESS_SIZE_UNDEFINED; >>> + break; >>> + } >>> + >>> + spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY; >> >> Can you guarantee that? Should probably be part of &info as well if addr is. > > Not now. > As per moving to info, see my comment about de-x86 of ACPI in U-Boot. I don't understand how describing your address space correctly has anything to do with de-x86'ing anything? x86 is the only (living) user of non-MMIO address space out there. > >>> + /* REVISIT: Hard coded values for now */ >>> + spcr->parity = 0; >>> + spcr->stop_bits = 1; >> >> IMHO those should be part of &info as well. If you have to hard code >> them, better hard code them in the driver rather than the generic ACPI >> generation code. > > Care to read cover letter carefully? Again, if you think your patch is terrible and shouldn't be applied anyway, please mark it as RFC. I'll be happy to wait with review until you've made up your mind then :). Alex
Hi Andy, On Fri, Nov 16, 2018 at 1:59 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1]. > Let's provide it in U-Boot. > > [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx > This URL redirects to https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table. It looks that the redirected one has a better name that deserves to be mentioned in the commit message :) > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > arch/x86/include/asm/acpi_table.h | 2 + > arch/x86/lib/acpi_table.c | 85 +++++++++++++++++++++++++++++++ > 2 files changed, 87 insertions(+) > [snip] Regards, Bin
On Sun, Nov 18, 2018 at 08:37:20PM +0800, Bin Meng wrote: > Hi Andy, > > On Fri, Nov 16, 2018 at 1:59 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1]. > > Let's provide it in U-Boot. > > > > [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx > > > > This URL redirects to > https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table. > It looks that the redirected one has a better name that deserves to be > mentioned in the commit message :) Sure, thanks!
On Thu, Nov 15, 2018 at 09:19:38PM +0100, Alexander Graf wrote: > On 15.11.18 20:43, Andy Shevchenko wrote: > > On Thu, Nov 15, 2018 at 8:27 PM Alexander Graf <agraf@suse.de> wrote: > >> On 15.11.18 18:58, Andy Shevchenko wrote: > >>> Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1]. > >>> Let's provide it in U-Boot. > >>> > >>> [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx Thanks for review.
diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h index 51cc806673..e3b65cff66 100644 --- a/arch/x86/include/asm/acpi_table.h +++ b/arch/x86/include/asm/acpi_table.h @@ -327,6 +327,8 @@ struct acpi_global_nvs; #define ACPI_DBG2_USB_XHCI 0x0000 #define ACPI_DBG2_USB_EHCI 0x0001 +#define ACPI_DBG2_UNKNOWN 0x00FF + /* SPCR (Serial Port Console Redirection table) */ struct __packed acpi_spcr { struct acpi_table_header header; diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c index c6b2026613..d8b44aea02 100644 --- a/arch/x86/lib/acpi_table.c +++ b/arch/x86/lib/acpi_table.c @@ -12,6 +12,7 @@ #include <cpu.h> #include <dm.h> #include <dm/uclass-internal.h> +#include <serial.h> #include <version.h> #include <asm/acpi/global_nvs.h> #include <asm/acpi_table.h> @@ -338,6 +339,82 @@ static void acpi_create_mcfg(struct acpi_mcfg *mcfg) header->checksum = table_compute_checksum((void *)mcfg, header->length); } +static void acpi_create_spcr(struct acpi_spcr *spcr) +{ + struct acpi_table_header *header = &(spcr->header); + struct serial_device_info info = {0}; + int access_size; + int ret; + + /* Fill out header fields */ + acpi_fill_header(header, "SPCR"); + header->length = sizeof(struct acpi_spcr); + header->revision = 2; + + ret = serial_getinfo(&info); + if (ret) + spcr->interface_type = ACPI_DBG2_UNKNOWN; + else + spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE; + debug("UART type %u (serial_getinfo() rc=%d)\n", spcr->interface_type, ret); + + /* Encode baud rate */ + switch (info.baudrate) { + case 9600: + spcr->baud_rate = 3; + break; + case 19200: + spcr->baud_rate = 4; + break; + case 57600: + spcr->baud_rate = 6; + break; + case 115200: + spcr->baud_rate = 7; + break; + default: + spcr->baud_rate = 0; + break; + } + + /* Encode register access size */ + switch (info.reg_shift) { + case 0: + access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS; + break; + case 1: + access_size = ACPI_ACCESS_SIZE_WORD_ACCESS; + break; + case 2: + access_size = ACPI_ACCESS_SIZE_DWORD_ACCESS; + break; + case 3: + access_size = ACPI_ACCESS_SIZE_QWORD_ACCESS; + break; + default: + access_size = ACPI_ACCESS_SIZE_UNDEFINED; + break; + } + + spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY; + spcr->serial_port.bit_width = info.reg_width; + spcr->serial_port.bit_offset = info.reg_offset; + spcr->serial_port.access_size = access_size; + spcr->serial_port.addrl = info.addr >> 0; + spcr->serial_port.addrh = info.addr >> 32; + + /* REVISIT: Hard coded values for now */ + spcr->parity = 0; + spcr->stop_bits = 1; + + /* REVISIT: No PCI devices for now */ + spcr->pci_device_id = 0xffff; + spcr->pci_vendor_id = 0xffff; + + /* Fix checksum */ + header->checksum = table_compute_checksum((void *)spcr, header->length); +} + /* * QEMU's version of write_acpi_tables is defined in drivers/misc/qfw.c */ @@ -352,6 +429,7 @@ ulong write_acpi_tables(ulong start) struct acpi_fadt *fadt; struct acpi_mcfg *mcfg; struct acpi_madt *madt; + struct acpi_spcr *spcr; int i; current = start; @@ -440,6 +518,13 @@ ulong write_acpi_tables(ulong start) acpi_add_table(rsdp, mcfg); current = ALIGN(current, 16); + debug("ACPI: * SPCR\n"); + spcr = (struct acpi_spcr *)current; + acpi_create_spcr(spcr); + current += spcr->header.length; + acpi_add_table(rsdp, spcr); + current = ALIGN(current, 16); + debug("current = %x\n", current); acpi_rsdp_addr = (unsigned long)rsdp;
Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1]. Let's provide it in U-Boot. [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- arch/x86/include/asm/acpi_table.h | 2 + arch/x86/lib/acpi_table.c | 85 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+)