All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: BALATON Zoltan <balaton@eik.bme.hu>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Richard Henderson <richard.henderson@linaro.org>,
	Magnus Damm <magnus.damm@gmail.com>,
	qemu-devel@nongnu.org,
	Yoshinori Sato <ysato@users.sourceforge.jp>
Subject: Re: [PATCH v4 06/23] hw/char/sh_serial: QOM-ify
Date: Fri, 29 Oct 2021 15:25:16 +0200	[thread overview]
Message-ID: <571d2e30-ad0a-b225-e3aa-ee96d847daa9@amsat.org> (raw)
In-Reply-To: <4198235-e93d-ddde-fb63-edfe9bd422f4@eik.bme.hu>

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


  reply	other threads:[~2021-10-29 13:26 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-28 19:27 [PATCH v4 00/23] More SH4 clean ups BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 04/23] hw/sh4/r2d: Use error_report instead of fprintf to stderr BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 22/23] hw/timer/sh_timer: Do not wrap lines that are not too long BALATON Zoltan
2021-10-29  5:48   ` Philippe Mathieu-Daudé
2021-10-28 19:27 ` [PATCH v4 13/23] hw/intc/sh_intc: Remove excessive parenthesis BALATON Zoltan
2021-10-29  5:39   ` Philippe Mathieu-Daudé
2021-10-28 19:27 ` [PATCH v4 20/23] hw/timer/sh_timer: Rename sh_timer_state to SHTimerState BALATON Zoltan
2021-10-29  5:47   ` Philippe Mathieu-Daudé
2021-10-28 19:27 ` [PATCH v4 21/23] hw/timer/sh_timer: Fix format strings and remove casts BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 18/23] hw/intc/sh_intc: Simplify allocating sources array BALATON Zoltan
2021-10-29  5:46   ` Philippe Mathieu-Daudé
2021-10-29 11:59     ` BALATON Zoltan
2021-10-29 12:41       ` Philippe Mathieu-Daudé
2021-10-28 19:27 ` [PATCH v4 15/23] hw/intc/sh_intc: Inline and drop sh_intc_source() function BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 17/23] hw/intc/sh_intc: Avoid using continue in loops BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 03/23] hw/sh4: Change debug printfs to traces BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 19/23] hw/intc/sh_intc: Remove unneeded local variable initialisers BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 23/23] hw/timer/sh_timer: Fix timer memory region size BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 11/23] hw/intc/sh_intc: Drop another useless macro BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 14/23] hw/intc/sh_intc: Use array index instead of pointer arithmetics BALATON Zoltan
2021-10-29  5:43   ` Philippe Mathieu-Daudé
2021-10-28 19:27 ` [PATCH v4 01/23] hw/sh4: Fix typos in a comment BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 16/23] hw/intc/sh_intc: Replace abort() with g_assert_not_reached() BALATON Zoltan
2021-10-29  5:41   ` Philippe Mathieu-Daudé
2021-10-28 19:27 ` [PATCH v4 08/23] hw/intc/sh_intc: Use existing macro instead of local one BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 05/23] hw/char/sh_serial: Rename type sh_serial_state to SHSerialState BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 12/23] hw/intc/sh_intc: Move sh_intc_register() closer to its only user BALATON Zoltan
2021-10-29  5:40   ` Philippe Mathieu-Daudé
2021-10-28 19:27 ` [PATCH v4 07/23] hw/char/sh_serial: Add device id to trace output BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 10/23] hw/intc/sh_intc: Rename iomem region BALATON Zoltan
2021-10-29  5:36   ` Philippe Mathieu-Daudé
2021-10-28 19:27 ` [PATCH v4 09/23] hw/intc/sh_intc: Turn some defines into an enum BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 02/23] hw/char/sh_serial: Use hw_error instead of fprintf and abort BALATON Zoltan
2021-10-29  5:38   ` Philippe Mathieu-Daudé
2021-10-29  6:00     ` Thomas Huth
2021-10-29 12:01     ` BALATON Zoltan
2021-10-28 19:27 ` [PATCH v4 06/23] hw/char/sh_serial: QOM-ify BALATON Zoltan
2021-10-29  6:05   ` Philippe Mathieu-Daudé
2021-10-29 12:15     ` BALATON Zoltan
2021-10-29 13:25       ` Philippe Mathieu-Daudé [this message]
2021-10-29 13:44         ` BALATON Zoltan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=571d2e30-ad0a-b225-e3aa-ee96d847daa9@amsat.org \
    --to=f4bug@amsat.org \
    --cc=balaton@eik.bme.hu \
    --cc=magnus.damm@gmail.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=ysato@users.sourceforge.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.