On 20.02.2020 08:44, Wei Xu wrote: > On 2020/2/17 21:53, Jan Beulich wrote: >> On 03.02.2020 12:21, Wei Xu wrote: >>> +static int __init ns16550_acpi_uart_init(const void *data) >>> +{ >>> + struct acpi_table_header *table; >>> + struct acpi_table_spcr *spcr; >>> + acpi_status status; >>> + /* >>> + * Same as the DT part. >>> + * Only support one UART on ARM which happen to be ns16550_com[0]. >>> + */ >>> + struct ns16550 *uart = &ns16550_com[0]; >>> + >>> + status = acpi_get_table(ACPI_SIG_SPCR, 0, &table); >>> + if ( ACPI_FAILURE(status) ) >>> + { >>> + printk("ns16550: Failed to get SPCR table\n"); >>> + return -EINVAL; >>> + } >>> + >>> + spcr = container_of(table, struct acpi_table_spcr, header); >>> + >>> + /* >>> + * The serial port address may be 0 for example >>> + * if the console redirection is disabled. >>> + */ >>> + if ( unlikely(!spcr->serial_port.address) ) >>> + { >>> + printk("ns16550: the serial port address is invalid\n"); >> >> Is zero really an invalid address, or is it rather a proper >> indicator of there not being any device? > > I will change the msg to "The console redirection is disabled." following the > description in the spcr. > Is that OK?
With the "The" preferably dropped, yes. >>> + return -EINVAL; >>> + } >>> + >>> + ns16550_init_common(uart); >>> + >>> + /* >>> + * The baud rate is pre-configured by the firmware. >> >> But this isn't the same as BAUD_AUTO, is it? If firmware pre-configures >> the baud rate, isn't it this structure which it would use to communicate >> the information? >> > > No, the comments of the BAUD_AUTO stated like that > and in fact the baud rate is not changed after the firmmware. Oh, I see. I should have checked the comment instead of implying meaning assigned to similarly named things elsewhere. Keep as is. >>> + */ >>> + uart->baud = BAUD_AUTO; >>> + uart->data_bits = 8; >>> + uart->parity = spcr->parity; >>> + uart->stop_bits = spcr->stop_bits; >>> + uart->io_base = spcr->serial_port.address; >>> + uart->io_size = spcr->serial_port.bit_width; >> >> Once again: You should not ignore the GAS address space indicator. > > Sorry, I did not get the point. > Do you mean check the address is 0 or not? No. I mean you shouldn't ignore other parts of the structure: struct acpi_generic_address { u8 space_id; /* Address space where struct or register exists */ u8 bit_width; /* Size in bits of given register */ u8 bit_offset; /* Bit offset within the register */ u8 access_width; /* Minimum Access size (ACPI 3.0) */ u64 address; /* 64-bit address of struct or register */ }; Iirc you now consume all fields except space_id, yet space_id is a qualifier to the address field (which you obviously use). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel