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);
 
 /* 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;
+    }
+    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);
+    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));
+}
+
 SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
 {
     SH7750State *s;
diff --git a/hw/sh4/Kconfig b/hw/sh4/Kconfig
index ab733a3f76..d23d5f5b1c 100644
--- a/hw/sh4/Kconfig
+++ b/hw/sh4/Kconfig
@@ -20,5 +20,5 @@ config SHIX
 config SH7750
     bool
     select SH_INTC
-    select SH_SCI
+    select RENESAS_SCI
     select SH_TIMER
-- 
2.20.1

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

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