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