[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/7] hw/char/cmsdk-apb-uart.c: Implement CMSDK A
From: |
Alistair Francis |
Subject: |
Re: [Qemu-devel] [PATCH 2/7] hw/char/cmsdk-apb-uart.c: Implement CMSDK APB UART |
Date: |
Tue, 11 Jul 2017 17:40:00 +0200 |
On Tue, Jul 11, 2017 at 5:33 PM, Peter Maydell <address@hidden> wrote:
> On 11 July 2017 at 16:12, Alistair Francis <address@hidden> wrote:
>> On Tue, Jul 11, 2017 at 1:17 PM, Peter Maydell <address@hidden> wrote:
>>> Implement a model of the simple "APB UART" provided in
>>> the Cortex-M System Design Kit (CMSDK).
>>>
>>> Signed-off-by: Peter Maydell <address@hidden>
>
>>> +} CMSDKAPBUART;
>>
>> This should be CamelCase.
>
> Yes, but CamelCase where all the words are all-uppercase
> (as with CMSDK, APB and UART) is indistinguishable from
> all-uppercase. (Compare the way we say "PCIIORegion" rather
> than "PciIoRegion".)
Good point, I feel like it just looks strange being all caps though.
>
>>> +/* This is a model of the "APB UART" which is part of the Cortex-M
>>> + * System Design Kit (CMSDK) and documented in the Cortex-M System
>>> + * Design Kit Technical Reference Manual (ARM DDI0479C):
>>> + *
>>> https://developer.arm.com/products/system-design/system-design-kits/cortex-m-system-design-kit
>>
>> I can't find the spec from this site. Is it possible to link directly
>> to the guides? I have found a few dead links from some of these
>> marketing focused site.
>
> It's the link with the big blue button "Cortex-M System Design Kit
> TRM". I didn't want to link directly to the TRM, because that is
> (currently) an infocenter.arm.com URL which may eventually go
> away as content migrates to developer.arm.com. (The DDI document
> reference number is unique and sufficient to find the document
> in future even in the face of broken links.)
>
>>> +static void uart_update_parameters(CMSDKAPBUART *s)
>>> +{
>>> + QEMUSerialSetParams ssp;
>>> +
>>> + /* This UART is always 8N1 but the baud rate is programmable.
>>> + * The minimum permitted bauddiv setting is 16, so we just ignore
>>> + * settings below that (usually this means the device has just
>>> + * been reset and not yet programmed).
>>> + */
>>> + if (s->bauddiv < 16 || s->bauddiv > s->pclk_frq) {
>>> + return;
>>> + }
>>
>> This seems like it should deserve a guest error print.
>
> As the comment says, that would cause us to print a spurious
> warning every time the device was reset.
I just see this being hard to debug. What about if not printing if
baud rate is 0 but guest error otherwise?
Or can this not be called from reset?
>
>>> +static int uart_can_receive(void *opaque)
>>> +{
>>> + CMSDKAPBUART *s = CMSDK_APB_UART(opaque);
>>> +
>>> + /* We can take a char if RX is enabled and the buffer is empty */
>>> + if (s->ctrl & R_CTRL_RX_EN_MASK && !(s->state & R_STATE_RXFULL_MASK)) {
>>> + return 1;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static void uart_receive(void *opaque, const uint8_t *buf, int size)
>>> +{
>>> + CMSDKAPBUART *s = CMSDK_APB_UART(opaque);
>>> +
>>> + trace_cmsdk_apb_uart_receive(*buf);
>>> +
>>> + if (!(s->ctrl & R_CTRL_RX_EN_MASK)) {
>>> + /* Just drop the character on the floor */
>>> + return;
>>
>> Doesn't this also deserve a guest error print.
>
> It's a can't-happen case because uart_can_receive() won't
> return 1 if the EN bit is clear. Checking again here is
> perhaps unnecessary paranoia, but it's not a guest error.
Ah good point. Maybe that is worth stating?
Thanks,
Alistair
>
>>> + }
>>> +
>>> + if (s->state & R_STATE_RXFULL_MASK) {
>>> + s->state |= R_STATE_RXOVERRUN_MASK;
>>> + }
>>> +
>>> + s->rxbuf = *buf;
>>> + s->state |= R_STATE_RXFULL_MASK;
>>> + if (s->ctrl & R_CTRL_RX_INTEN_MASK) {
>>> + s->intstatus |= R_INTSTATUS_RX_MASK;
>>> + }
>>> + cmsdk_apb_uart_update(s);
>>> +}
>>> +
>>> +static uint64_t uart_read(void *opaque, hwaddr offset, unsigned size)
>>> +{
>>> + CMSDKAPBUART *s = CMSDK_APB_UART(opaque);
>>> + uint64_t r;
>>> +
>>> + switch (offset) {
>>> + case A_DATA:
>>> + r = s->rxbuf;
>>> + s->state &= ~R_STATE_RXFULL_MASK;
>>> + cmsdk_apb_uart_update(s);
>>> + break;
>>> + case A_STATE:
>>> + r = s->state;
>>> + break;
>>> + case A_CTRL:
>>> + r = s->ctrl;
>>> + break;
>>> + case A_INTSTATUS:
>>> + r = s->intstatus;
>>> + break;
>>> + case A_BAUDDIV:
>>> + r = s->bauddiv;
>>> + break;
>>
>> You used pasrt of the register API but not the second part. This seems
>> like a greaet device to use the register API on.
>
> I really don't like the register API. The field definition
> convenience macros are fine, but I prefer to define devices
> with read and write functions with switches, I think it's
> clearer, and it's easier to see where you want to put a breakpoint
> to be able to step through register reads and writes, and so on.
>
>>> + case A_PID4 ... A_CID3:
>>> + r = uart_id[offset / 4 - A_PID4];
>>
>> I think this is the one you already found in the cover letter.
>
> Yep. (Same in both timer and uart.)
>
>>> + switch (offset) {
>>> + case A_DATA:
>>> + {
>>> + s->txbuf = value;
>>> + if (s->state & R_STATE_TXFULL_MASK) {
>>> + /* Buffer already full -- note the overrun and let the
>>> + * existing pending transmit callback handle the new char.
>>> + */
>>> + s->state |= R_STATE_TXOVERRUN_MASK;
>>> + cmsdk_apb_uart_update(s);
>>> + } else {
>>> + s->state |= R_STATE_TXFULL_MASK;
>>> + uart_transmit(NULL, G_IO_OUT, s);
>>> + }
>>> + break;
>>> + }
>>
>> Why is this case inside braces?
>
> I probably had a local variable in there at one point which
> I ended up not needing. I'll delete the braces.
>
> thanks
> -- PMM
Re: [Qemu-devel] [PATCH 2/7] hw/char/cmsdk-apb-uart.c: Implement CMSDK APB UART, Philippe Mathieu-Daudé, 2017/07/11
Re: [Qemu-devel] [PATCH 0/7] ARM: implement MPS2 board (with 2 FPGA flavours), Philippe Mathieu-Daudé, 2017/07/11
Re: [Qemu-devel] [PATCH 0/7] ARM: implement MPS2 board (with 2 FPGA flavours), no-reply, 2017/07/11
Re: [Qemu-devel] [PATCH 0/7] ARM: implement MPS2 board (with 2 FPGA flavours), no-reply, 2017/07/11