From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>, Wei Xu <xuwei5@hisilicon.com>
Cc: sstabellini@kernel.org, Wei Liu <wl@xen.org>,
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
George Dunlap <George.Dunlap@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>,
linuxarm@huawei.com, shameerali.kolothum.thodi@huawei.com,
prime.zeng@hisilicon.com, xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH] ns16550: Add ACPI support
Date: Sat, 18 Jan 2020 12:32:37 +0000 [thread overview]
Message-ID: <d936960f-214d-788b-29cf-7be147332ea9@xen.org> (raw)
In-Reply-To: <539d5900-1cc6-a490-7319-5357c6aa1219@suse.com>
Hi Jan,
On 17/01/2020 08:33, Jan Beulich wrote:
> On 17.01.2020 04:40, Wei Xu wrote:
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>> DT_DEVICE_END
>>
>> #endif /* HAS_DEVICE_TREE */
>> +
>> +#ifdef CONFIG_ACPI
>> +#include <xen/acpi.h>
>> +
>> +static int __init ns16550_acpi_uart_init(const void *data)
>> +{
>> + struct acpi_table_spcr *spcr = NULL;
>
> The initializer isn't strictly needed, is it?
>
>> + acpi_status status;
>> + struct ns16550 *uart;
>> +
>> + status = acpi_get_table(ACPI_SIG_SPCR, 0,
>> + (struct acpi_table_header **)&spcr);
>> +
>> + if ( ACPI_FAILURE(status) )
>> + {
>> + printk("ns16550: Failed to get SPCR table\n");
>> + return -EINVAL;
>> + }
>> +
>> + uart = &ns16550_com[0];
>
> You want to justify the choice of what (on x86 at least= we'd call
> com1 in the patch description. Also this could be the initializer
> of the variable.
This is the same choice as we made for the DT binding (see
ns16550_uart()). We only support one UART on Arm which happen to be
ns16550_com[0] (but named diferrently).
The code below is actually quite similar to the DT parsing, so maybe we
want to provide a common helper here.
>
>> + ns16550_init_common(uart);
>> +
>> + uart->baud = BAUD_AUTO;
>
> There's a baud_rate field in the structure. If there's a reason
> to ignore it, please add a comment.
Same as for the DT part, we assume the firmware will configure the UART
correctly.
>
> There's also an interface_type field - can you really ignore it?
It is not ignored. This is used by the upper layer to detect which uart
driver to call (see acpi_uart_init() in arm-uart.c).
>
>> + uart->data_bits = 8;
>> + uart->parity = spcr->parity;
>> + uart->stop_bits = spcr->stop_bits;
>
> There's also a flow_control field, which I think needs checking
> that it matches ns16550_setup_preirq() comment:
>
> /* No flow ctrl: DTR and RTS are both wedged high to keep remote happy. */
>
> Similarly any other fields you don't evaluate at all and which
> aren't explained by the spec as possible to be ignored (and the
> situation matching the use case, like you not caring about PCI
> aspects here) need reasoning about in the description or a code
> comment.
What's missing in the commit message is the fact this is only targeting
Arm. So there are a lot we don't care yet (such as PCI).
>
>> + uart->io_base = spcr->serial_port.address;
>
> The field (or perhaps the whole spcr->serial_port) being zero looks
> to have special meaning.
>
>> + uart->io_size = 8;
>> + uart->reg_shift = spcr->serial_port.bit_offset;
>
> spcr->serial_port has other fields which I don't think you should
> ignore.
>
>> + uart->reg_width = 1;
>
> Please use consistent placement of = : Either all of them are
> aligned, or all of them are preceded by a single space only.
>
>> + /* trigger/polarity information is not available in spcr */
>> + irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
>> + uart->irq = spcr->interrupt;
>> +
>> + uart->vuart.base_addr = uart->io_base;
>> + uart->vuart.size = uart->io_size;
>> + uart->vuart.data_off = UART_THR << uart->reg_shift;
>> + uart->vuart.status_off = UART_LSR << uart->reg_shift;
>> + uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT;
>
> Style-wise this block should then match whatever the other
> block above looks.
>
>> + /* Register with generic serial driver. */
>> + serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
>> +
>> + return 0;
>> +}
>> +
>> +ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
>> + .class_type = ACPI_DBG2_16550_COMPATIBLE,
>> + .init = ns16550_acpi_uart_init,
>> +ACPI_DEVICE_END
>
> I don't expect this to build on x86.
This is only meant to target Arm. So maybe we want to protect the whole
code with defined(CONFIG_ACPI) && defined(CONFIG_ARM).
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2020-01-18 12:33 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-17 3:40 [Xen-devel] [PATCH] ns16550: Add ACPI support Wei Xu
2020-01-17 8:33 ` Jan Beulich
2020-01-18 12:32 ` Julien Grall [this message]
2020-01-20 8:38 ` Jan Beulich
2020-01-21 3:28 ` Wei Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d936960f-214d-788b-29cf-7be147332ea9@xen.org \
--to=julien@xen.org \
--cc=George.Dunlap@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=konrad.wilk@oracle.com \
--cc=linuxarm@huawei.com \
--cc=prime.zeng@hisilicon.com \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
--cc=xuwei5@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).