On Tue, 29 Jun 2021 22:23:01 +0900,
Peter Maydell wrote:
>
> 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.
I think option B is fine.
I would like to change sh7750 to qom as the next step.
I don't want to make any major changes at this step.
> > /* 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 ?
Yes.
This array used old changes. I forgot remove it.
>
> 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.
OK.
> 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.
OK.
> > + 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.
It was before checking with checkpatch.pl.
I will clean it in the next patch.
> > + 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
--
Yoshinori Sato
