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

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 9544 bytes --]

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