On Wed, 16 Jun 2021 at 10:14, Yoshinori Sato <ys...@users.sourceforge.jp> wrote:
>
> Signed-off-by: Yoshinori Sato <ys...@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
