Re: [PATCH 2/8] tiva c usart module implementation

[Peter Maydell]

On Wed, 17 May 2023 at 09:13, Mohamed ElSayed <m.elsayed4420@gmail.com> wrote:
>
> Signed-off-by: Mohamed ElSayed <m.elsayed4420@gmail.com>
> ---
>  hw/char/tm4c123_usart.c         | 381 ++++++++++++++++++++++++++++++++
>  hw/char/trace-events            |   4 +
>  include/hw/char/tm4c123_usart.h | 124 +++++++++++

Patches that add new device source files should also have
the changes to the Kconfig and meson.build files that tie
them in to the build system (which you currently have put
all together in patch 8).

>  3 files changed, 509 insertions(+)
>  create mode 100644 hw/char/tm4c123_usart.c
>  create mode 100644 include/hw/char/tm4c123_usart.h
>
> diff --git a/hw/char/tm4c123_usart.c b/hw/char/tm4c123_usart.c
> new file mode 100644
> index 0000000000..21bfe781b0
> --- /dev/null
> +++ b/hw/char/tm4c123_usart.c
> @@ -0,0 +1,381 @@
> +/*
> + * TM4C123 USART
> + *
> + * Copyright (c) 2023 Mohamed ElSayed <m.elsayed4420@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */

TI don't explicitly say so, but this UART is a variant of the PL011.
So I think instead of a completely separate model for it, we should
extend the hw/char/pl011.c code to handle any specific new behaviour
we need for this variant. There's already handling in pl011.c
for the TYPE_PL011_LUMINARY, which is an older TI-specific PL011
flavour, so you have a pattern to work with. For that variant the
only thing we needed to override was the ID register values; you
might need a little bit more for this one, but likely not much.

So below I've only noted a few things rather than doing a review
of the whole device.

> +#include "qemu/osdep.h"
> +#include "hw/char/tm4c123_usart.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "trace.h"
> +
> +#define LOG(mask, fmt, args...) qemu_log_mask(mask, "%s: " fmt, __func__, ## 
> args)

Please don't hide simple calls to qemu_log_mask() behind
macros like this.

> +#define READONLY LOG(LOG_GUEST_ERROR, "0x%"HWADDR_PRIx" is a readonly 
> field\n.", addr)

If you put all the readonly registers together and let them
fall through, ie.
   case USART_FR:
   case USART_RIS:
       /* etc */
       qemu_log_mask(...);

then you only need one line which reports the write to a read-only
register, and the macro isn't really necessary.

(Also, stray trailing '.' after the newline.)


> +
> +static bool usart_clock_enabled(TM4C123SysCtlState *s, hwaddr addr)
> +{
> +    switch (addr) {
> +        case USART_0:
> +            return s->sysctl_rcgcuart & (1 << 0);
> +            break;
> +        case USART_1:
> +            return s->sysctl_rcgcuart & (1 << 1);
> +            break;
> +        case USART_2:
> +            return s->sysctl_rcgcuart & (1 << 2);
> +            break;
> +        case USART_3:
> +            return s->sysctl_rcgcuart & (1 << 3);
> +            break;
> +        case USART_4:
> +            return s->sysctl_rcgcuart & (1 << 4);
> +            break;
> +        case USART_5:
> +            return s->sysctl_rcgcuart & (1 << 5);
> +            break;
> +        case USART_6:
> +            return s->sysctl_rcgcuart & (1 << 6);
> +            break;
> +        case USART_7:
> +            return s->sysctl_rcgcuart & (1 << 7);
> +            break;
> +    }
> +    return false;
> +}

The UART device shouldn't have a direct pointer to the sysctl
device like this, and it shouldn't be poking around inside
its MMIORegion to find out its physical address either.

The "right" approach here is that the sysctl device has a
bunch of clock outputs for the various UART clocks, the SoC
code wires up each clock output to the appropriate UART
device, and the UART device calls clock_is_enabled() on
its input clock.

The simple approach is to say "we'll just assume the
UART clock has been enabled", because correct guest code
will do that anyway and the device implementation doesn't
much care what the clock is. That's what we usually do
with UART models.

thanks
-- PMM