Re: [PATCH 3/3] hw/sh4: sh7750 using renesas_sci.

[Peter Maydell <peter.maydell@linaro.org>]

On Wed, 16 Jun 2021 at 10:14, Yoshinori Sato <ysato@users.sourceforge.jp> wrote:
>
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  include/hw/sh4/sh.h |  8 --------
>  hw/sh4/sh7750.c     | 41 +++++++++++++++++++++++++++++++++++++++++
>  hw/sh4/Kconfig      |  2 +-
>  3 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/include/hw/sh4/sh.h b/include/hw/sh4/sh.h
> index becb596979..74e1ba59a8 100644
> --- a/include/hw/sh4/sh.h
> +++ b/include/hw/sh4/sh.h
> @@ -55,14 +55,6 @@ int sh7750_register_io_device(struct SH7750State *s,
>
>  /* sh_serial.c */
>  #define SH_SERIAL_FEAT_SCIF (1 << 0)
> -void sh_serial_init(MemoryRegion *sysmem,
> -                    hwaddr base, int feat,
> -                    uint32_t freq, Chardev *chr,
> -                    qemu_irq eri_source,
> -                    qemu_irq rxi_source,
> -                    qemu_irq txi_source,
> -                    qemu_irq tei_source,
> -                    qemu_irq bri_source);

This change means that the code in sh_serial.c will no longer compile,
because it has a non-static function with no previous prototype.
The patch as a whole compiles because the change to hw/sh4/Kconfig
file removes the selection of SH_SCI and so we never try to
compile sh_serial.c. But we shouldn't leave dead code around in
the tree.

Option A:
I guess the idea is to avoid having to rename sh_serial_init(),
which makes sense. If you want to take this route, then
in this patch we should mention that in the commit message,
something like:

===begin===
hw/sh4: Switch sh7750 to new renesas-sci devices

Switch the sh7750 away from the old serial device implementation
in sh_serial.c to use the new renesas-sci devices.

Note that deleting the prototype for sh_serial_init() means that
sh_serial.c will no longer be able to compile (it would hit a
warning about having a non-static function without a previous
prototype). This is OK because we remove the only place that
was selecting the SH_SCI Kconfig feature, so we won't attempt
to compile that source file. In the following commit, we will
delete the file entirely.

===end===

Then in a new patch to go after this one, remove:
 * the file sh_serial.c itself
 * the "config SH_SCI" section in hw/char/Kconfig
 * the line for sh_serial.c in hw/char/meson.build


Option B: would be to give your new sh7750.c function a different
name than sh_serial_init() (eg sci_init()) and change the two
callsites in this patch. Then you could keep the old sh_serial_init()
prototype in this patch and delete it as part of the new "remove
sh_serial.c" patch (which is where removal of the prototype more
logically belongs.)

I don't mind which of the two you go for.

>  /* sh7750.c */
>  qemu_irq sh7750_irl(struct SH7750State *s);
> diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
> index d53a436d8c..1ef8b73c65 100644
> --- a/hw/sh4/sh7750.c
> +++ b/hw/sh4/sh7750.c
> @@ -24,6 +24,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "hw/irq.h"
>  #include "hw/sh4/sh.h"
>  #include "sysemu/sysemu.h"
> @@ -32,6 +33,8 @@
>  #include "hw/sh4/sh_intc.h"
>  #include "hw/timer/tmu012.h"
>  #include "exec/exec-all.h"
> +#include "hw/char/renesas_sci.h"
> +#include "hw/qdev-properties.h"
>
>  #define NB_DEVICES 4
>
> @@ -752,6 +755,44 @@ static const MemoryRegionOps sh7750_mmct_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> +static void sh_serial_init(MemoryRegion *sysmem,
> +                           hwaddr base, int feat,
> +                           uint32_t freq, Chardev *chr,
> +                           qemu_irq eri_source,
> +                           qemu_irq rxi_source,
> +                           qemu_irq txi_source,
> +                           qemu_irq tei_source,
> +                           qemu_irq bri_source)
> +{
> +    RenesasSCIBaseState *sci;
> +    char ckname[16];
> +
> +    switch(feat) {
> +    case 0: /* SCI */
> +        sci = RENESAS_SCI_BASE(qdev_new(TYPE_RENESAS_SCI));
> +        snprintf(ckname, sizeof(ckname), "pck_sci");
> +        break;
> +    case SH_SERIAL_FEAT_SCIF:
> +        sci = RENESAS_SCI_BASE(qdev_new(TYPE_RENESAS_SCIF));
> +        snprintf(ckname, sizeof(ckname), "pck_scif");
> +        break;
> +    }

The ckname[] array seems to be set but never used ?

Since you never use 'sci' as a RenesasSCIBaseState, you could
instead declare

   Device *sci;

and then
   sci = qdev_new(TYPE_RENESAS_whatever);

and avoid some of the DEVICE() casts below.

You might also prefer to have a SysBusDevice *sbd which you
can then set with
   sbd = SYS_BUS_DEVICE(sci);
and use sbd instead of casting every time you need it.

> +    qdev_prop_set_chr(DEVICE(sci), "chardev", chr);
> +    qdev_prop_set_uint32(DEVICE(sci), "register-size", SCI_REGWIDTH_32);
> +    qdev_prop_set_uint64(DEVICE(sci), "input-freq", freq);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(sci), 0, eri_source);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(sci), 1, rxi_source);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(sci), 2, txi_source);
> +    if (tei_source)
> +        sysbus_connect_irq(SYS_BUS_DEVICE(sci), 3, tei_source);
> +    if (bri_source)
> +        sysbus_connect_irq(SYS_BUS_DEVICE(sci), 3, bri_source);

QEMU coding style requires braces for all if statements, even when
they're only one line.

> +    sysbus_realize(SYS_BUS_DEVICE(sci), &error_abort);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(sci), 0, base);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(sci), 1, P4ADDR(base));
> +    sysbus_mmio_map(SYS_BUS_DEVICE(sci), 2, A7ADDR(base));
> +}

thanks
-- PMM