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

[Philippe Mathieu-Daudé]

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).