[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] ns16550: Add ACPI support for ARM only
Hi Jan, On 2020/2/20 16:33, Jan Beulich wrote: > 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. > Got it. >>>> + 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. > Got it. >>>> + */ >>>> + 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). I did not know what the space_id means yet. I will investigate it. Thanks a lot! Best Regards, Wei > > Jan > > . > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |