diff mbox series

[v4,06/23] hw/char/sh_serial: QOM-ify

Message ID 3208b60a33e3452b832089aecd5c1aed800eb6bc.1635449225.git.balaton@eik.bme.hu
State New
Headers show
Series More SH4 clean ups | expand

Commit Message

BALATON Zoltan Oct. 28, 2021, 7:27 p.m. UTC 
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/char/sh_serial.c | 107 +++++++++++++++++++++++++++-----------------
 hw/sh4/sh7750.c     |  62 ++++++++++++++++++-------
 include/hw/sh4/sh.h |   9 +---
 3 files changed, 114 insertions(+), 64 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 29, 2021, 6:05 a.m. UTC | #1 
On 10/28/21 21:27, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/char/sh_serial.c | 107 +++++++++++++++++++++++++++-----------------
>  hw/sh4/sh7750.c     |  62 ++++++++++++++++++-------
>  include/hw/sh4/sh.h |   9 +---
>  3 files changed, 114 insertions(+), 64 deletions(-)

> +OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
> +
> +struct SHSerialState {
> +    SysBusDevice parent;
[...]
> -} SHSerialState;
> +};
> +
> +typedef struct {} SHSerialStateClass;

OBJECT_DECLARE_TYPE()?

> -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)
> +static void sh_serial_reset(DeviceState *dev)

Can you extract sh_serial_reset() in a previous patch?

>  {
> -    SHSerialState *s = g_malloc0(sizeof(*s));
> +    SHSerialState *s = SH_SERIAL(dev);
>  
> -    s->feat = feat;
>      s->flags = SH_SERIAL_FLAG_TEND | SH_SERIAL_FLAG_TDE;
>      s->rtrg = 1;
>  
> @@ -397,38 +396,64 @@ void sh_serial_init(MemoryRegion *sysmem,
>      s->scr = 1 << 5; /* pretend that TX is enabled so early printk works */
>      s->sptr = 0;
>  
> -    if (feat & SH_SERIAL_FEAT_SCIF) {
> +    if (s->feat & SH_SERIAL_FEAT_SCIF) {
>          s->fcr = 0;
>      } else {
>          s->dr = 0xff;
>      }
>  
>      sh_serial_clear_fifo(s);
> +}
>  
> -    memory_region_init_io(&s->iomem, NULL, &sh_serial_ops, s,
> -                          "serial", 0x100000000ULL);

Keep that, ...

> -    memory_region_init_alias(&s->iomem_p4, NULL, "serial-p4", &s->iomem,
> -                             0, 0x28);
> -    memory_region_add_subregion(sysmem, P4ADDR(base), &s->iomem_p4);
> -
> -    memory_region_init_alias(&s->iomem_a7, NULL, "serial-a7", &s->iomem,
> -                             0, 0x28);
> -    memory_region_add_subregion(sysmem, A7ADDR(base), &s->iomem_a7);

... and these lines become one single sysbus_init_mmio() ...

> -
> -    if (chr) {
> -        qemu_chr_fe_init(&s->chr, chr, &error_abort);
> +static void sh_serial_realize(DeviceState *d, Error **errp)
> +{
> +    SHSerialState *s = SH_SERIAL(d);
> +    MemoryRegion *iomem = g_malloc(sizeof(*iomem));
> +
> +    assert(d->id);
> +    memory_region_init_io(iomem, OBJECT(d), &sh_serial_ops, s, d->id, 0x28);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(d), iomem);
> +    qdev_init_gpio_out_named(d, &s->eri, "eri", 1);
> +    qdev_init_gpio_out_named(d, &s->rxi, "rxi", 1);
> +    qdev_init_gpio_out_named(d, &s->txi, "txi", 1);
> +    qdev_init_gpio_out_named(d, &s->tei, "tei", 1);
> +    qdev_init_gpio_out_named(d, &s->bri, "bri", 1);
> +
> +    if (qemu_chr_fe_backend_connected(&s->chr)) {
>          qemu_chr_fe_set_handlers(&s->chr, sh_serial_can_receive1,
>                                   sh_serial_receive1,
>                                   sh_serial_event, NULL, s, NULL, true);
>      }
>  
> -    s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> -                                         sh_serial_timeout_int, s);
> +    timer_init_ns(&s->fifo_timeout_timer, QEMU_CLOCK_VIRTUAL,
> +                  sh_serial_timeout_int, s);
>      s->etu = NANOSECONDS_PER_SECOND / 9600;
> -    s->eri = eri_source;
> -    s->rxi = rxi_source;
> -    s->txi = txi_source;
> -    s->tei = tei_source;
> -    s->bri = bri_source;
> +}

> @@ -762,6 +766,9 @@ static const MemoryRegionOps sh7750_mmct_ops = {
>  SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
>  {
>      SH7750State *s;
> +    DeviceState *dev;
> +    SysBusDevice *sb;
> +    MemoryRegion *mr, *alias;
>  
>      s = g_malloc0(sizeof(SH7750State));
>      s->cpu = cpu;
> @@ -807,21 +814,46 @@ SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
>  
>      cpu->env.intc_handle = &s->intc;
>  
> -    sh_serial_init(sysmem, 0x1fe00000,
> -                   0, s->periph_freq, serial_hd(0),
> -                   s->intc.irqs[SCI1_ERI],
> -                   s->intc.irqs[SCI1_RXI],
> -                   s->intc.irqs[SCI1_TXI],
> -                   s->intc.irqs[SCI1_TEI],
> -                   NULL);
> -    sh_serial_init(sysmem, 0x1fe80000,
> -                   SH_SERIAL_FEAT_SCIF,
> -                   s->periph_freq, serial_hd(1),
> -                   s->intc.irqs[SCIF_ERI],
> -                   s->intc.irqs[SCIF_RXI],
> -                   s->intc.irqs[SCIF_TXI],
> -                   NULL,
> -                   s->intc.irqs[SCIF_BRI]);
> +    /* SCI */
> +    dev = qdev_new(TYPE_SH_SERIAL);
> +    dev->id = (char *)"sci";
> +    qdev_prop_set_chr(dev, "chardev", serial_hd(0));
> +    sb = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(sb, &error_fatal);
> +    mr = sysbus_mmio_get_region(sb, 0);
> +    alias = g_malloc(sizeof(*alias));
> +    memory_region_init_alias(alias, OBJECT(dev), "sci-p4", mr,
> +                             0, memory_region_size(mr));
> +    memory_region_add_subregion(sysmem, P4ADDR(0x1fe00000), alias);
> +    alias = g_malloc(sizeof(*alias));
> +    memory_region_init_alias(alias, OBJECT(dev), "sci-a7", mr,
> +                             0, memory_region_size(mr));
> +    memory_region_add_subregion(sysmem, A7ADDR(0x1fe00000), alias);

... then you can replace the aliases by 2 sysbus_mmio_map() calls.

> +    qdev_connect_gpio_out_named(dev, "eri", 0, s->intc.irqs[SCI1_ERI]);
> +    qdev_connect_gpio_out_named(dev, "rxi", 0, s->intc.irqs[SCI1_RXI]);
> +    qdev_connect_gpio_out_named(dev, "txi", 0, s->intc.irqs[SCI1_TXI]);
> +    qdev_connect_gpio_out_named(dev, "tei", 0, s->intc.irqs[SCI1_TEI]);
> +
> +    /* SCIF */
> +    dev = qdev_new(TYPE_SH_SERIAL);
> +    dev->id =  (char *)"scif";
> +    qdev_prop_set_chr(dev, "chardev", serial_hd(1));
> +    qdev_prop_set_uint8(dev, "features", SH_SERIAL_FEAT_SCIF);
> +    sb = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(sb, &error_fatal);
> +    mr = sysbus_mmio_get_region(sb, 0);
> +    alias = g_malloc(sizeof(*alias));
> +    memory_region_init_alias(alias, OBJECT(dev), "scif-p4", mr,
> +                             0, memory_region_size(mr));
> +    memory_region_add_subregion(sysmem, P4ADDR(0x1fe80000), alias);
> +    alias = g_malloc(sizeof(*alias));
> +    memory_region_init_alias(alias, OBJECT(dev), "scif-a7", mr,
> +                             0, memory_region_size(mr));
> +    memory_region_add_subregion(sysmem, A7ADDR(0x1fe80000), alias);
> +    qdev_connect_gpio_out_named(dev, "eri", 0, s->intc.irqs[SCIF_ERI]);
> +    qdev_connect_gpio_out_named(dev, "rxi", 0, s->intc.irqs[SCIF_RXI]);
> +    qdev_connect_gpio_out_named(dev, "txi", 0, s->intc.irqs[SCIF_TXI]);
> +    qdev_connect_gpio_out_named(dev, "bri", 0, s->intc.irqs[SCIF_BRI]);
>
BALATON Zoltan Oct. 29, 2021, 12:15 p.m. UTC | #2 
On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/28/21 21:27, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/char/sh_serial.c | 107 +++++++++++++++++++++++++++-----------------
>>  hw/sh4/sh7750.c     |  62 ++++++++++++++++++-------
>>  include/hw/sh4/sh.h |   9 +---
>>  3 files changed, 114 insertions(+), 64 deletions(-)
>
>> +OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
>> +
>> +struct SHSerialState {
>> +    SysBusDevice parent;
> [...]
>> -} SHSerialState;
>> +};
>> +
>> +typedef struct {} SHSerialStateClass;
>
> OBJECT_DECLARE_TYPE()?

From include/qom/object.h:
  * OBJECT_DECLARE_SIMPLE_TYPE:
[...]
  * This does the same as OBJECT_DECLARE_TYPE(), but with no class struct
  * declared.
  *
  * This macro should be used unless the class struct needs to have
  * virtual methods declared.

I think we're rather missing OBJECT_DEFINE_SIMPLE_TYPE. A lot of current 
object definitions are open coded because of that and could be replaced if 
we had that simple variant but we don't, so this is the shortest way for 
now.

>> -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)
>> +static void sh_serial_reset(DeviceState *dev)
>
> Can you extract sh_serial_reset() in a previous patch?

I could.

>>  {
>> -    SHSerialState *s = g_malloc0(sizeof(*s));
>> +    SHSerialState *s = SH_SERIAL(dev);
>>
>> -    s->feat = feat;
>>      s->flags = SH_SERIAL_FLAG_TEND | SH_SERIAL_FLAG_TDE;
>>      s->rtrg = 1;
>>
>> @@ -397,38 +396,64 @@ void sh_serial_init(MemoryRegion *sysmem,
>>      s->scr = 1 << 5; /* pretend that TX is enabled so early printk works */
>>      s->sptr = 0;
>>
>> -    if (feat & SH_SERIAL_FEAT_SCIF) {
>> +    if (s->feat & SH_SERIAL_FEAT_SCIF) {
>>          s->fcr = 0;
>>      } else {
>>          s->dr = 0xff;
>>      }
>>
>>      sh_serial_clear_fifo(s);
>> +}
>>
>> -    memory_region_init_io(&s->iomem, NULL, &sh_serial_ops, s,
>> -                          "serial", 0x100000000ULL);
>
> Keep that, ...
>
>> -    memory_region_init_alias(&s->iomem_p4, NULL, "serial-p4", &s->iomem,
>> -                             0, 0x28);
>> -    memory_region_add_subregion(sysmem, P4ADDR(base), &s->iomem_p4);
>> -
>> -    memory_region_init_alias(&s->iomem_a7, NULL, "serial-a7", &s->iomem,
>> -                             0, 0x28);
>> -    memory_region_add_subregion(sysmem, A7ADDR(base), &s->iomem_a7);
>
> ... and these lines become one single sysbus_init_mmio() ...

Not sure about that. The device doesn't really have two io regions, they 
just appear twice due to how the CPU maps them. So I'd keep a single MMIO 
region here but could map one directly and use only one alias for the 
other instead. (That would get rid of either serial-a7 or serial-p4 with 
the other just called serial or actually sci/scif after this series).

Regards,
BALATON Zoltan

>> -
>> -    if (chr) {
>> -        qemu_chr_fe_init(&s->chr, chr, &error_abort);
>> +static void sh_serial_realize(DeviceState *d, Error **errp)
>> +{
>> +    SHSerialState *s = SH_SERIAL(d);
>> +    MemoryRegion *iomem = g_malloc(sizeof(*iomem));
>> +
>> +    assert(d->id);
>> +    memory_region_init_io(iomem, OBJECT(d), &sh_serial_ops, s, d->id, 0x28);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(d), iomem);
>> +    qdev_init_gpio_out_named(d, &s->eri, "eri", 1);
>> +    qdev_init_gpio_out_named(d, &s->rxi, "rxi", 1);
>> +    qdev_init_gpio_out_named(d, &s->txi, "txi", 1);
>> +    qdev_init_gpio_out_named(d, &s->tei, "tei", 1);
>> +    qdev_init_gpio_out_named(d, &s->bri, "bri", 1);
>> +
>> +    if (qemu_chr_fe_backend_connected(&s->chr)) {
>>          qemu_chr_fe_set_handlers(&s->chr, sh_serial_can_receive1,
>>                                   sh_serial_receive1,
>>                                   sh_serial_event, NULL, s, NULL, true);
>>      }
>>
>> -    s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> -                                         sh_serial_timeout_int, s);
>> +    timer_init_ns(&s->fifo_timeout_timer, QEMU_CLOCK_VIRTUAL,
>> +                  sh_serial_timeout_int, s);
>>      s->etu = NANOSECONDS_PER_SECOND / 9600;
>> -    s->eri = eri_source;
>> -    s->rxi = rxi_source;
>> -    s->txi = txi_source;
>> -    s->tei = tei_source;
>> -    s->bri = bri_source;
>> +}
>
>> @@ -762,6 +766,9 @@ static const MemoryRegionOps sh7750_mmct_ops = {
>>  SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
>>  {
>>      SH7750State *s;
>> +    DeviceState *dev;
>> +    SysBusDevice *sb;
>> +    MemoryRegion *mr, *alias;
>>
>>      s = g_malloc0(sizeof(SH7750State));
>>      s->cpu = cpu;
>> @@ -807,21 +814,46 @@ SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
>>
>>      cpu->env.intc_handle = &s->intc;
>>
>> -    sh_serial_init(sysmem, 0x1fe00000,
>> -                   0, s->periph_freq, serial_hd(0),
>> -                   s->intc.irqs[SCI1_ERI],
>> -                   s->intc.irqs[SCI1_RXI],
>> -                   s->intc.irqs[SCI1_TXI],
>> -                   s->intc.irqs[SCI1_TEI],
>> -                   NULL);
>> -    sh_serial_init(sysmem, 0x1fe80000,
>> -                   SH_SERIAL_FEAT_SCIF,
>> -                   s->periph_freq, serial_hd(1),
>> -                   s->intc.irqs[SCIF_ERI],
>> -                   s->intc.irqs[SCIF_RXI],
>> -                   s->intc.irqs[SCIF_TXI],
>> -                   NULL,
>> -                   s->intc.irqs[SCIF_BRI]);
>> +    /* SCI */
>> +    dev = qdev_new(TYPE_SH_SERIAL);
>> +    dev->id = (char *)"sci";
>> +    qdev_prop_set_chr(dev, "chardev", serial_hd(0));
>> +    sb = SYS_BUS_DEVICE(dev);
>> +    sysbus_realize_and_unref(sb, &error_fatal);
>> +    mr = sysbus_mmio_get_region(sb, 0);
>> +    alias = g_malloc(sizeof(*alias));
>> +    memory_region_init_alias(alias, OBJECT(dev), "sci-p4", mr,
>> +                             0, memory_region_size(mr));
>> +    memory_region_add_subregion(sysmem, P4ADDR(0x1fe00000), alias);
>> +    alias = g_malloc(sizeof(*alias));
>> +    memory_region_init_alias(alias, OBJECT(dev), "sci-a7", mr,
>> +                             0, memory_region_size(mr));
>> +    memory_region_add_subregion(sysmem, A7ADDR(0x1fe00000), alias);
>
> ... then you can replace the aliases by 2 sysbus_mmio_map() calls.
>
>> +    qdev_connect_gpio_out_named(dev, "eri", 0, s->intc.irqs[SCI1_ERI]);
>> +    qdev_connect_gpio_out_named(dev, "rxi", 0, s->intc.irqs[SCI1_RXI]);
>> +    qdev_connect_gpio_out_named(dev, "txi", 0, s->intc.irqs[SCI1_TXI]);
>> +    qdev_connect_gpio_out_named(dev, "tei", 0, s->intc.irqs[SCI1_TEI]);
>> +
>> +    /* SCIF */
>> +    dev = qdev_new(TYPE_SH_SERIAL);
>> +    dev->id =  (char *)"scif";
>> +    qdev_prop_set_chr(dev, "chardev", serial_hd(1));
>> +    qdev_prop_set_uint8(dev, "features", SH_SERIAL_FEAT_SCIF);
>> +    sb = SYS_BUS_DEVICE(dev);
>> +    sysbus_realize_and_unref(sb, &error_fatal);
>> +    mr = sysbus_mmio_get_region(sb, 0);
>> +    alias = g_malloc(sizeof(*alias));
>> +    memory_region_init_alias(alias, OBJECT(dev), "scif-p4", mr,
>> +                             0, memory_region_size(mr));
>> +    memory_region_add_subregion(sysmem, P4ADDR(0x1fe80000), alias);
>> +    alias = g_malloc(sizeof(*alias));
>> +    memory_region_init_alias(alias, OBJECT(dev), "scif-a7", mr,
>> +                             0, memory_region_size(mr));
>> +    memory_region_add_subregion(sysmem, A7ADDR(0x1fe80000), alias);
>> +    qdev_connect_gpio_out_named(dev, "eri", 0, s->intc.irqs[SCIF_ERI]);
>> +    qdev_connect_gpio_out_named(dev, "rxi", 0, s->intc.irqs[SCIF_RXI]);
>> +    qdev_connect_gpio_out_named(dev, "txi", 0, s->intc.irqs[SCIF_TXI]);
>> +    qdev_connect_gpio_out_named(dev, "bri", 0, s->intc.irqs[SCIF_BRI]);
>>
>
>
Philippe Mathieu-Daudé Oct. 29, 2021, 1:25 p.m. UTC | #3 
On 10/29/21 14:15, BALATON Zoltan wrote:
> On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/28/21 21:27, BALATON Zoltan wrote:
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/char/sh_serial.c | 107 +++++++++++++++++++++++++++-----------------
>>>  hw/sh4/sh7750.c     |  62 ++++++++++++++++++-------
>>>  include/hw/sh4/sh.h |   9 +---
>>>  3 files changed, 114 insertions(+), 64 deletions(-)
>>
>>> +OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
>>> +
>>> +struct SHSerialState {
>>> +    SysBusDevice parent;
>> [...]
>>> -} SHSerialState;
>>> +};
>>> +
>>> +typedef struct {} SHSerialStateClass;
>>
>> OBJECT_DECLARE_TYPE()?
> 
> From include/qom/object.h:
>  * OBJECT_DECLARE_SIMPLE_TYPE:
> [...]
>  * This does the same as OBJECT_DECLARE_TYPE(), but with no class struct
>  * declared.
>  *
>  * This macro should be used unless the class struct needs to have
>  * virtual methods declared.
> 
> I think we're rather missing OBJECT_DEFINE_SIMPLE_TYPE. A lot of current
> object definitions are open coded because of that and could be replaced
> if we had that simple variant but we don't, so this is the shortest way
> for now.
> 
>>> -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)
>>> +static void sh_serial_reset(DeviceState *dev)
>>
>> Can you extract sh_serial_reset() in a previous patch?
> 
> I could.
> 
>>>  {
>>> -    SHSerialState *s = g_malloc0(sizeof(*s));
>>> +    SHSerialState *s = SH_SERIAL(dev);
>>>
>>> -    s->feat = feat;
>>>      s->flags = SH_SERIAL_FLAG_TEND | SH_SERIAL_FLAG_TDE;
>>>      s->rtrg = 1;
>>>
>>> @@ -397,38 +396,64 @@ void sh_serial_init(MemoryRegion *sysmem,
>>>      s->scr = 1 << 5; /* pretend that TX is enabled so early printk
>>> works */
>>>      s->sptr = 0;
>>>
>>> -    if (feat & SH_SERIAL_FEAT_SCIF) {
>>> +    if (s->feat & SH_SERIAL_FEAT_SCIF) {
>>>          s->fcr = 0;
>>>      } else {
>>>          s->dr = 0xff;
>>>      }
>>>
>>>      sh_serial_clear_fifo(s);
>>> +}
>>>
>>> -    memory_region_init_io(&s->iomem, NULL, &sh_serial_ops, s,
>>> -                          "serial", 0x100000000ULL);
>>
>> Keep that, ...
>>
>>> -    memory_region_init_alias(&s->iomem_p4, NULL, "serial-p4",
>>> &s->iomem,
>>> -                             0, 0x28);
>>> -    memory_region_add_subregion(sysmem, P4ADDR(base), &s->iomem_p4);
>>> -
>>> -    memory_region_init_alias(&s->iomem_a7, NULL, "serial-a7",
>>> &s->iomem,
>>> -                             0, 0x28);
>>> -    memory_region_add_subregion(sysmem, A7ADDR(base), &s->iomem_a7);
>>
>> ... and these lines become one single sysbus_init_mmio() ...
> 
> Not sure about that. The device doesn't really have two io regions, they
> just appear twice due to how the CPU maps them. So I'd keep a single
> MMIO region here but could map one directly and use only one alias for
> the other instead. (That would get rid of either serial-a7 or serial-p4
> with the other just called serial or actually sci/scif after this series).

Looking at the current mapping:

memory-region: system
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-0000000000ffffff (prio 0, romd): r2d.flash
    0000000004000000-000000000400003f (prio 0, i/o): r2d-fpga
    000000000c000000-000000000fffffff (prio 0, ram): r2d.sdram
    0000000010000000-00000000107fffff (prio 0, ram): sm501.local
    0000000013e00000-0000000013ffffff (prio 0, i/o): sm501.mmio
      0000000013e00000-0000000013e0006b (prio 0, i/o): sm501-system-config
      0000000013e10040-0000000013e10053 (prio 0, i/o): sm501-i2c
      0000000013e30000-0000000013e3001f (prio 0, i/o): serial
      0000000013e40000-0000000013e400ff (prio 0, i/o): ohci
      0000000013e80000-0000000013e80fff (prio 0, i/o): sm501-disp-ctrl
      0000000013f00000-0000000013f00053 (prio 0, i/o): sm501-2d-engine
    000000001400080c-000000001400080f (prio 0, i/o): ide-mmio.2
    0000000014001000-000000001400101f (prio 0, i/o): ide-mmio.1
    000000001e080000-000000001e080003 (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001e080000-000000001e080003
    000000001e080040-000000001e080043 (prio 0, i/o): alias
interrupt-controller-mask-set-a7 @interrupt-controller
000000001e080040-000000001e080043
    000000001e080060-000000001e080063 (prio 0, i/o): alias
interrupt-controller-mask-clr-a7 @interrupt-controller
000000001e080060-000000001e080063
    000000001e100000-000000001e100fff (prio 0, i/o): alias timer-a7
@timer 0000000000000000-0000000000000fff
    000000001e200000-000000001e200223 (prio 0, i/o): alias sh_pci.2
@sh_pci 0000000000000000-0000000000000223
    000000001f000000-000000001f000fff (prio 0, i/o): alias memory-1f0
@memory 000000001f000000-000000001f000fff
    000000001f800000-000000001f800fff (prio 0, i/o): alias memory-1f8
@memory 000000001f800000-000000001f800fff
    000000001fc00000-000000001fc00fff (prio 0, i/o): alias memory-1fc
@memory 000000001fc00000-000000001fc00fff
    000000001fd00004-000000001fd00007 (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001fd00004-000000001fd00007
    000000001fd00008-000000001fd0000b (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001fd00008-000000001fd0000b
    000000001fd0000c-000000001fd0000f (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001fd0000c-000000001fd0000f
    000000001fd00010-000000001fd00013 (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001fd00010-000000001fd00013
    000000001fd80000-000000001fd80fff (prio 0, i/o): alias timer-a7
@timer 0000000000000000-0000000000000fff
    000000001fe00000-000000001fe00027 (prio 0, i/o): alias serial-a7
@serial 0000000000000000-0000000000000027
    000000001fe80000-000000001fe80027 (prio 0, i/o): alias serial-a7
@serial 0000000000000000-0000000000000027
    00000000f0000000-00000000f7ffffff (prio 0, i/o): cache-and-tlb
    00000000fe080000-00000000fe080003 (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001e080000-000000001e080003
    00000000fe080040-00000000fe080043 (prio 0, i/o): alias
interrupt-controller-mask-set-p4 @interrupt-controller
000000001e080040-000000001e080043
    00000000fe080060-00000000fe080063 (prio 0, i/o): alias
interrupt-controller-mask-clr-p4 @interrupt-controller
000000001e080060-000000001e080063
    00000000fe100000-00000000fe100fff (prio 0, i/o): alias timer-p4
@timer 0000000000000000-0000000000000fff
    00000000fe200000-00000000fe200223 (prio 0, i/o): sh_pci
    00000000fe240000-00000000fe27ffff (prio 0, i/o): alias sh_pci.isa
@io 0000000000000000-000000000003ffff
    00000000ff000000-00000000ff000fff (prio 0, i/o): alias memory-ff0
@memory 000000001f000000-000000001f000fff
    00000000ff800000-00000000ff800fff (prio 0, i/o): alias memory-ff8
@memory 000000001f800000-000000001f800fff
    00000000ffc00000-00000000ffc00fff (prio 0, i/o): alias memory-ffc
@memory 000000001fc00000-000000001fc00fff
    00000000ffd00004-00000000ffd00007 (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001fd00004-000000001fd00007
    00000000ffd00008-00000000ffd0000b (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001fd00008-000000001fd0000b
    00000000ffd0000c-00000000ffd0000f (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001fd0000c-000000001fd0000f
    00000000ffd00010-00000000ffd00013 (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001fd00010-000000001fd00013
    00000000ffd80000-00000000ffd80fff (prio 0, i/o): alias timer-p4
@timer 0000000000000000-0000000000000fff
    00000000ffe00000-00000000ffe00027 (prio 0, i/o): alias serial-p4
@serial 0000000000000000-0000000000000027
    00000000ffe80000-00000000ffe80027 (prio 0, i/o): alias serial-p4
@serial 0000000000000000-0000000000000027

It seems the 32MiB container region in 0x1e000000-0x1fffffff is
aliased to 0xfe000000-0xffffffff. But I haven't looked at the
datasheet (and don't have time until next week).
BALATON Zoltan Oct. 29, 2021, 1:44 p.m. UTC | #4 
On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/29/21 14:15, BALATON Zoltan wrote:
>> On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
>>> On 10/28/21 21:27, BALATON Zoltan wrote:
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/char/sh_serial.c | 107 +++++++++++++++++++++++++++-----------------
>>>>  hw/sh4/sh7750.c     |  62 ++++++++++++++++++-------
>>>>  include/hw/sh4/sh.h |   9 +---
>>>>  3 files changed, 114 insertions(+), 64 deletions(-)
>>>
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
>>>> +
>>>> +struct SHSerialState {
>>>> +    SysBusDevice parent;
>>> [...]
>>>> -} SHSerialState;
>>>> +};
>>>> +
>>>> +typedef struct {} SHSerialStateClass;
>>>
>>> OBJECT_DECLARE_TYPE()?
>>
>> From include/qom/object.h:
>>  * OBJECT_DECLARE_SIMPLE_TYPE:
>> [...]
>>  * This does the same as OBJECT_DECLARE_TYPE(), but with no class struct
>>  * declared.
>>  *
>>  * This macro should be used unless the class struct needs to have
>>  * virtual methods declared.
>>
>> I think we're rather missing OBJECT_DEFINE_SIMPLE_TYPE. A lot of current
>> object definitions are open coded because of that and could be replaced
>> if we had that simple variant but we don't, so this is the shortest way
>> for now.
>>
>>>> -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)
>>>> +static void sh_serial_reset(DeviceState *dev)
>>>
>>> Can you extract sh_serial_reset() in a previous patch?
>>
>> I could.
>>
>>>>  {
>>>> -    SHSerialState *s = g_malloc0(sizeof(*s));
>>>> +    SHSerialState *s = SH_SERIAL(dev);
>>>>
>>>> -    s->feat = feat;
>>>>      s->flags = SH_SERIAL_FLAG_TEND | SH_SERIAL_FLAG_TDE;
>>>>      s->rtrg = 1;
>>>>
>>>> @@ -397,38 +396,64 @@ void sh_serial_init(MemoryRegion *sysmem,
>>>>      s->scr = 1 << 5; /* pretend that TX is enabled so early printk
>>>> works */
>>>>      s->sptr = 0;
>>>>
>>>> -    if (feat & SH_SERIAL_FEAT_SCIF) {
>>>> +    if (s->feat & SH_SERIAL_FEAT_SCIF) {
>>>>          s->fcr = 0;
>>>>      } else {
>>>>          s->dr = 0xff;
>>>>      }
>>>>
>>>>      sh_serial_clear_fifo(s);
>>>> +}
>>>>
>>>> -    memory_region_init_io(&s->iomem, NULL, &sh_serial_ops, s,
>>>> -                          "serial", 0x100000000ULL);
>>>
>>> Keep that, ...
>>>
>>>> -    memory_region_init_alias(&s->iomem_p4, NULL, "serial-p4",
>>>> &s->iomem,
>>>> -                             0, 0x28);
>>>> -    memory_region_add_subregion(sysmem, P4ADDR(base), &s->iomem_p4);
>>>> -
>>>> -    memory_region_init_alias(&s->iomem_a7, NULL, "serial-a7",
>>>> &s->iomem,
>>>> -                             0, 0x28);
>>>> -    memory_region_add_subregion(sysmem, A7ADDR(base), &s->iomem_a7);
>>>
>>> ... and these lines become one single sysbus_init_mmio() ...
>>
>> Not sure about that. The device doesn't really have two io regions, they
>> just appear twice due to how the CPU maps them. So I'd keep a single
>> MMIO region here but could map one directly and use only one alias for
>> the other instead. (That would get rid of either serial-a7 or serial-p4
>> with the other just called serial or actually sci/scif after this series).
>
> Looking at the current mapping:
>
> memory-region: system
>  0000000000000000-ffffffffffffffff (prio 0, i/o): system
>    0000000000000000-0000000000ffffff (prio 0, romd): r2d.flash
>    0000000004000000-000000000400003f (prio 0, i/o): r2d-fpga
>    000000000c000000-000000000fffffff (prio 0, ram): r2d.sdram
>    0000000010000000-00000000107fffff (prio 0, ram): sm501.local
>    0000000013e00000-0000000013ffffff (prio 0, i/o): sm501.mmio
>      0000000013e00000-0000000013e0006b (prio 0, i/o): sm501-system-config
>      0000000013e10040-0000000013e10053 (prio 0, i/o): sm501-i2c
>      0000000013e30000-0000000013e3001f (prio 0, i/o): serial
>      0000000013e40000-0000000013e400ff (prio 0, i/o): ohci
>      0000000013e80000-0000000013e80fff (prio 0, i/o): sm501-disp-ctrl
>      0000000013f00000-0000000013f00053 (prio 0, i/o): sm501-2d-engine
>    000000001400080c-000000001400080f (prio 0, i/o): ide-mmio.2
>    0000000014001000-000000001400101f (prio 0, i/o): ide-mmio.1
>    000000001e080000-000000001e080003 (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001e080000-000000001e080003
>    000000001e080040-000000001e080043 (prio 0, i/o): alias
> interrupt-controller-mask-set-a7 @interrupt-controller
> 000000001e080040-000000001e080043
>    000000001e080060-000000001e080063 (prio 0, i/o): alias
> interrupt-controller-mask-clr-a7 @interrupt-controller
> 000000001e080060-000000001e080063
>    000000001e100000-000000001e100fff (prio 0, i/o): alias timer-a7
> @timer 0000000000000000-0000000000000fff
>    000000001e200000-000000001e200223 (prio 0, i/o): alias sh_pci.2
> @sh_pci 0000000000000000-0000000000000223
>    000000001f000000-000000001f000fff (prio 0, i/o): alias memory-1f0
> @memory 000000001f000000-000000001f000fff
>    000000001f800000-000000001f800fff (prio 0, i/o): alias memory-1f8
> @memory 000000001f800000-000000001f800fff
>    000000001fc00000-000000001fc00fff (prio 0, i/o): alias memory-1fc
> @memory 000000001fc00000-000000001fc00fff
>    000000001fd00004-000000001fd00007 (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001fd00004-000000001fd00007
>    000000001fd00008-000000001fd0000b (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001fd00008-000000001fd0000b
>    000000001fd0000c-000000001fd0000f (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001fd0000c-000000001fd0000f
>    000000001fd00010-000000001fd00013 (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001fd00010-000000001fd00013
>    000000001fd80000-000000001fd80fff (prio 0, i/o): alias timer-a7
> @timer 0000000000000000-0000000000000fff
>    000000001fe00000-000000001fe00027 (prio 0, i/o): alias serial-a7
> @serial 0000000000000000-0000000000000027
>    000000001fe80000-000000001fe80027 (prio 0, i/o): alias serial-a7
> @serial 0000000000000000-0000000000000027
>    00000000f0000000-00000000f7ffffff (prio 0, i/o): cache-and-tlb
>    00000000fe080000-00000000fe080003 (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001e080000-000000001e080003
>    00000000fe080040-00000000fe080043 (prio 0, i/o): alias
> interrupt-controller-mask-set-p4 @interrupt-controller
> 000000001e080040-000000001e080043
>    00000000fe080060-00000000fe080063 (prio 0, i/o): alias
> interrupt-controller-mask-clr-p4 @interrupt-controller
> 000000001e080060-000000001e080063
>    00000000fe100000-00000000fe100fff (prio 0, i/o): alias timer-p4
> @timer 0000000000000000-0000000000000fff
>    00000000fe200000-00000000fe200223 (prio 0, i/o): sh_pci
>    00000000fe240000-00000000fe27ffff (prio 0, i/o): alias sh_pci.isa
> @io 0000000000000000-000000000003ffff
>    00000000ff000000-00000000ff000fff (prio 0, i/o): alias memory-ff0
> @memory 000000001f000000-000000001f000fff
>    00000000ff800000-00000000ff800fff (prio 0, i/o): alias memory-ff8
> @memory 000000001f800000-000000001f800fff
>    00000000ffc00000-00000000ffc00fff (prio 0, i/o): alias memory-ffc
> @memory 000000001fc00000-000000001fc00fff
>    00000000ffd00004-00000000ffd00007 (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001fd00004-000000001fd00007
>    00000000ffd00008-00000000ffd0000b (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001fd00008-000000001fd0000b
>    00000000ffd0000c-00000000ffd0000f (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001fd0000c-000000001fd0000f
>    00000000ffd00010-00000000ffd00013 (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001fd00010-000000001fd00013
>    00000000ffd80000-00000000ffd80fff (prio 0, i/o): alias timer-p4
> @timer 0000000000000000-0000000000000fff
>    00000000ffe00000-00000000ffe00027 (prio 0, i/o): alias serial-p4
> @serial 0000000000000000-0000000000000027
>    00000000ffe80000-00000000ffe80027 (prio 0, i/o): alias serial-p4
> @serial 0000000000000000-0000000000000027
>
> It seems the 32MiB container region in 0x1e000000-0x1fffffff is
> aliased to 0xfe000000-0xffffffff. But I haven't looked at the
> datasheet (and don't have time until next week).

All regs are available at two addresses. The P regions are similar to 
MIPS' ksegs, the A7 is only accessible via MMU while P4 is I think only in 
protected mode but I also don't know all the details so I'd just leave it 
as it is for now. This could be cleaned up later in separete patches, I 
think there will be more clean ups needed as I've also found missing 
functionality that I plan to look at later but I'd like the clean ups 
merged before the freeze at least so I have less patches to roll in my 
tree.

Regards,
BALATON Zoltan
diff mbox series

Patch

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index b93b403555..0af5d7a228 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -27,7 +27,11 @@ 
 
 #include "qemu/osdep.h"
 #include "hw/hw.h"
+#include "hw/sysbus.h"
 #include "hw/irq.h"
+#include "hw/qdev-core.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "hw/sh4/sh.h"
 #include "chardev/char-fe.h"
 #include "qapi/error.h"
@@ -42,10 +46,10 @@ 
 
 #define SH_RX_FIFO_LENGTH (16)
 
-typedef struct {
-    MemoryRegion iomem;
-    MemoryRegion iomem_p4;
-    MemoryRegion iomem_a7;
+OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
+
+struct SHSerialState {
+    SysBusDevice parent;
     uint8_t smr;
     uint8_t brr;
     uint8_t scr;
@@ -59,13 +63,12 @@  typedef struct {
     uint8_t rx_tail;
     uint8_t rx_head;
 
-    int freq;
-    int feat;
+    uint8_t feat;
     int flags;
     int rtrg;
 
     CharBackend chr;
-    QEMUTimer *fifo_timeout_timer;
+    QEMUTimer fifo_timeout_timer;
     uint64_t etu; /* Elementary Time Unit (ns) */
 
     qemu_irq eri;
@@ -73,7 +76,11 @@  typedef struct {
     qemu_irq txi;
     qemu_irq tei;
     qemu_irq bri;
-} SHSerialState;
+};
+
+typedef struct {} SHSerialStateClass;
+
+OBJECT_DEFINE_TYPE(SHSerialState, sh_serial, SH_SERIAL, SYS_BUS_DEVICE)
 
 static void sh_serial_clear_fifo(SHSerialState *s)
 {
@@ -349,11 +356,11 @@  static void sh_serial_receive1(void *opaque, const uint8_t *buf, int size)
                 if (s->rx_cnt >= s->rtrg) {
                     s->flags |= SH_SERIAL_FLAG_RDF;
                     if (s->scr & (1 << 6) && s->rxi) {
-                        timer_del(s->fifo_timeout_timer);
+                        timer_del(&s->fifo_timeout_timer);
                         qemu_set_irq(s->rxi, 1);
                     }
                 } else {
-                    timer_mod(s->fifo_timeout_timer,
+                    timer_mod(&s->fifo_timeout_timer,
                         qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 15 * s->etu);
                 }
             }
@@ -377,18 +384,10 @@  static const MemoryRegionOps sh_serial_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-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)
+static void sh_serial_reset(DeviceState *dev)
 {
-    SHSerialState *s = g_malloc0(sizeof(*s));
+    SHSerialState *s = SH_SERIAL(dev);
 
-    s->feat = feat;
     s->flags = SH_SERIAL_FLAG_TEND | SH_SERIAL_FLAG_TDE;
     s->rtrg = 1;
 
@@ -397,38 +396,64 @@  void sh_serial_init(MemoryRegion *sysmem,
     s->scr = 1 << 5; /* pretend that TX is enabled so early printk works */
     s->sptr = 0;
 
-    if (feat & SH_SERIAL_FEAT_SCIF) {
+    if (s->feat & SH_SERIAL_FEAT_SCIF) {
         s->fcr = 0;
     } else {
         s->dr = 0xff;
     }
 
     sh_serial_clear_fifo(s);
+}
 
-    memory_region_init_io(&s->iomem, NULL, &sh_serial_ops, s,
-                          "serial", 0x100000000ULL);
-
-    memory_region_init_alias(&s->iomem_p4, NULL, "serial-p4", &s->iomem,
-                             0, 0x28);
-    memory_region_add_subregion(sysmem, P4ADDR(base), &s->iomem_p4);
-
-    memory_region_init_alias(&s->iomem_a7, NULL, "serial-a7", &s->iomem,
-                             0, 0x28);
-    memory_region_add_subregion(sysmem, A7ADDR(base), &s->iomem_a7);
-
-    if (chr) {
-        qemu_chr_fe_init(&s->chr, chr, &error_abort);
+static void sh_serial_realize(DeviceState *d, Error **errp)
+{
+    SHSerialState *s = SH_SERIAL(d);
+    MemoryRegion *iomem = g_malloc(sizeof(*iomem));
+
+    assert(d->id);
+    memory_region_init_io(iomem, OBJECT(d), &sh_serial_ops, s, d->id, 0x28);
+    sysbus_init_mmio(SYS_BUS_DEVICE(d), iomem);
+    qdev_init_gpio_out_named(d, &s->eri, "eri", 1);
+    qdev_init_gpio_out_named(d, &s->rxi, "rxi", 1);
+    qdev_init_gpio_out_named(d, &s->txi, "txi", 1);
+    qdev_init_gpio_out_named(d, &s->tei, "tei", 1);
+    qdev_init_gpio_out_named(d, &s->bri, "bri", 1);
+
+    if (qemu_chr_fe_backend_connected(&s->chr)) {
         qemu_chr_fe_set_handlers(&s->chr, sh_serial_can_receive1,
                                  sh_serial_receive1,
                                  sh_serial_event, NULL, s, NULL, true);
     }
 
-    s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
-                                         sh_serial_timeout_int, s);
+    timer_init_ns(&s->fifo_timeout_timer, QEMU_CLOCK_VIRTUAL,
+                  sh_serial_timeout_int, s);
     s->etu = NANOSECONDS_PER_SECOND / 9600;
-    s->eri = eri_source;
-    s->rxi = rxi_source;
-    s->txi = txi_source;
-    s->tei = tei_source;
-    s->bri = bri_source;
+}
+
+static void sh_serial_finalize(Object *obj)
+{
+    SHSerialState *s = SH_SERIAL(obj);
+
+    timer_del(&s->fifo_timeout_timer);
+}
+
+static void sh_serial_init(Object *obj)
+{
+}
+
+static Property sh_serial_properties[] = {
+    DEFINE_PROP_CHR("chardev", SHSerialState, chr),
+    DEFINE_PROP_UINT8("features", SHSerialState, feat, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void sh_serial_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    device_class_set_props(dc, sh_serial_properties);
+    dc->realize = sh_serial_realize;
+    dc->reset = sh_serial_reset;
+    /* Reason: part of SuperH CPU/SoC, needs to be wired up */
+    dc->user_creatable = false;
 }
diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
index 6c702d627c..22016de664 100644
--- a/hw/sh4/sh7750.c
+++ b/hw/sh4/sh7750.c
@@ -24,9 +24,13 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/sysbus.h"
 #include "hw/irq.h"
 #include "hw/sh4/sh.h"
 #include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "sh7750_regs.h"
 #include "sh7750_regnames.h"
 #include "hw/sh4/sh_intc.h"
@@ -762,6 +766,9 @@  static const MemoryRegionOps sh7750_mmct_ops = {
 SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
 {
     SH7750State *s;
+    DeviceState *dev;
+    SysBusDevice *sb;
+    MemoryRegion *mr, *alias;
 
     s = g_malloc0(sizeof(SH7750State));
     s->cpu = cpu;
@@ -807,21 +814,46 @@  SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
 
     cpu->env.intc_handle = &s->intc;
 
-    sh_serial_init(sysmem, 0x1fe00000,
-                   0, s->periph_freq, serial_hd(0),
-                   s->intc.irqs[SCI1_ERI],
-                   s->intc.irqs[SCI1_RXI],
-                   s->intc.irqs[SCI1_TXI],
-                   s->intc.irqs[SCI1_TEI],
-                   NULL);
-    sh_serial_init(sysmem, 0x1fe80000,
-                   SH_SERIAL_FEAT_SCIF,
-                   s->periph_freq, serial_hd(1),
-                   s->intc.irqs[SCIF_ERI],
-                   s->intc.irqs[SCIF_RXI],
-                   s->intc.irqs[SCIF_TXI],
-                   NULL,
-                   s->intc.irqs[SCIF_BRI]);
+    /* SCI */
+    dev = qdev_new(TYPE_SH_SERIAL);
+    dev->id = (char *)"sci";
+    qdev_prop_set_chr(dev, "chardev", serial_hd(0));
+    sb = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(sb, &error_fatal);
+    mr = sysbus_mmio_get_region(sb, 0);
+    alias = g_malloc(sizeof(*alias));
+    memory_region_init_alias(alias, OBJECT(dev), "sci-p4", mr,
+                             0, memory_region_size(mr));
+    memory_region_add_subregion(sysmem, P4ADDR(0x1fe00000), alias);
+    alias = g_malloc(sizeof(*alias));
+    memory_region_init_alias(alias, OBJECT(dev), "sci-a7", mr,
+                             0, memory_region_size(mr));
+    memory_region_add_subregion(sysmem, A7ADDR(0x1fe00000), alias);
+    qdev_connect_gpio_out_named(dev, "eri", 0, s->intc.irqs[SCI1_ERI]);
+    qdev_connect_gpio_out_named(dev, "rxi", 0, s->intc.irqs[SCI1_RXI]);
+    qdev_connect_gpio_out_named(dev, "txi", 0, s->intc.irqs[SCI1_TXI]);
+    qdev_connect_gpio_out_named(dev, "tei", 0, s->intc.irqs[SCI1_TEI]);
+
+    /* SCIF */
+    dev = qdev_new(TYPE_SH_SERIAL);
+    dev->id =  (char *)"scif";
+    qdev_prop_set_chr(dev, "chardev", serial_hd(1));
+    qdev_prop_set_uint8(dev, "features", SH_SERIAL_FEAT_SCIF);
+    sb = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(sb, &error_fatal);
+    mr = sysbus_mmio_get_region(sb, 0);
+    alias = g_malloc(sizeof(*alias));
+    memory_region_init_alias(alias, OBJECT(dev), "scif-p4", mr,
+                             0, memory_region_size(mr));
+    memory_region_add_subregion(sysmem, P4ADDR(0x1fe80000), alias);
+    alias = g_malloc(sizeof(*alias));
+    memory_region_init_alias(alias, OBJECT(dev), "scif-a7", mr,
+                             0, memory_region_size(mr));
+    memory_region_add_subregion(sysmem, A7ADDR(0x1fe80000), alias);
+    qdev_connect_gpio_out_named(dev, "eri", 0, s->intc.irqs[SCIF_ERI]);
+    qdev_connect_gpio_out_named(dev, "rxi", 0, s->intc.irqs[SCIF_RXI]);
+    qdev_connect_gpio_out_named(dev, "txi", 0, s->intc.irqs[SCIF_TXI]);
+    qdev_connect_gpio_out_named(dev, "bri", 0, s->intc.irqs[SCIF_BRI]);
 
     tmu012_init(sysmem, 0x1fd80000,
                 TMU012_FEAT_TOCR | TMU012_FEAT_3CHAN | TMU012_FEAT_EXTCLK,
diff --git a/include/hw/sh4/sh.h b/include/hw/sh4/sh.h
index 366cedcda0..ec716cdd45 100644
--- a/include/hw/sh4/sh.h
+++ b/include/hw/sh4/sh.h
@@ -54,15 +54,8 @@  int sh7750_register_io_device(struct SH7750State *s,
                               sh7750_io_device *device);
 
 /* sh_serial.c */
+#define TYPE_SH_SERIAL "sh-serial"
 #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);