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

[Yoshinori Sato]

On Tue, 29 Jun 2021 22:23:01 +0900,
Peter Maydell wrote:
> 
> 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.

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