| Message ID | 20210616091244.33049-4-ysato@users.sourceforge.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | renesas_sci update | expand |
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
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
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(-)