[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation
From: |
BB |
Subject: |
Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation |
Date: |
Mon, 29 Aug 2022 20:07:26 +0200 |
User-agent: |
K-9 Mail for Android |
Am 29. August 2022 19:50:10 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 29 Aug 2022, BB wrote:
>> Am 24. August 2022 01:23:14 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>> On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>> ---
>>>>>> hw/isa/vt82c686.c | 12 +++++++++++-
>>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>> index 47f2fd2669..ee745d5d49 100644
>>>>>> --- a/hw/isa/vt82c686.c
>>>>>> +++ b/hw/isa/vt82c686.c
>>>>>> @@ -546,6 +546,7 @@ struct ViaISAState {
>>>>>> qemu_irq cpu_intr;
>>>>>> qemu_irq *isa_irqs;
>>>>>> ViaSuperIOState via_sio;
>>>>>> + RTCState rtc;
>>>>>> PCIIDEState ide;
>>>>>> UHCIState uhci[2];
>>>>>> ViaPMState pm;
>>>>>> @@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
>>>>>> {
>>>>>> ViaISAState *s = VIA_ISA(obj);
>>>>>>
>>>>>> + object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>>>> object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>>>> object_initialize_child(obj, "uhci1", &s->uhci[0],
>>>>> "vt82c686b-usb-uhci");
>>>>>> object_initialize_child(obj, "uhci2", &s->uhci[1],
>>>>> "vt82c686b-usb-uhci");
>>>>>> @@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error
>>>>> **errp)
>>>>>> isa_bus_irqs(isa_bus, s->isa_irqs);
>>>>>> i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>>>> i8257_dma_init(isa_bus, 0);
>>>>>> - mc146818_rtc_init(isa_bus, 2000, NULL);
>>>>>> +
>>>>>> + /* RTC */
>>>>>> + qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>>>>> + if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>>>>> + return;
>>>>>> + }
>>>>>> + object_property_add_alias(qdev_get_machine(), "rtc-time",
>>>>> OBJECT(&s->rtc),
>>>>>> + "date");
>>>>>> + isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
>>>>>>
>>>>>> for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>>>>>> if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
>>>>>>
>>>>>
>>>>> This actually introduces code duplication as all other places except piix4
>>>>> seem to still use the init function (probably to ensure that the rtc-rime
>>>>> alias on the machine is properly set) so I'd keep this the same as
>>>>> everything else and drop this patch until this init function is removed
>>>>> from all other places as well.
>>>>>
>>>>
>>>> Hi Zoltan,
>>>>
>>>> Thanks for the fast reply! Regarding code homogeneity and duplication I've
>>>> made a similar argument for mc146818_rtc_init() in the past [1] and I've
>>>> learnt that my patch went backwards. Incidentally, Peter mentioned vt686c
>>>> as a candidate for the embed-the-device-struct style which - again
>>>> incidentally - I've now done.
>>>
>>> I've seen patches embedding devices recently but in this case it looked not
>>> that simple because of the rtc-time alias.
>>>
>>>> The rtc-time alias is actually only used by a couple of PPC machines where
>>>> Pegasos II is one of them. So the alias actually needs to be created only
>>>> for these machines, and identifying the cases where it has to be preserved
>>>> requires a lot of careful investigation. In the Pegasos II case this seems
>>>> especially complicated since one needs to look through several layers of
>>>> devices. During my work on the VT82xx south bridges I've gained some
>>>> knowledge such that I'd like to make this simplifying contribution.
>>>
>>> I've used it to implement the get-time-of-day rtas call with VOF in
>>> pegasos2 because otherwise it would need to access internals of the RTC
>>> model and/or duplicate some code. Here's the message discussing this:
>>>
>>> https://lists.nongnu.org/archive/html/qemu-ppc/2021-10/msg00170.html
>>>
>>> so this alias still seems to be the simplest way.
>>>
>>> I think the primary function of this alias is not these ppc machines but
>>> some QMP/HMP command or to make the guest time available from the monitor
>>> or something like that so it's probably also used from there and therefore
>>> all rtc probably should have it but I'm not sure about it.
>>
>> Indeed, the alias seems to be a convenience for some QMP/HMP commands.
>> AFAICS only the mc146818 sets the alias while it is probably not the only
>> RTC modelled in QEMU. So I wonder why boards using another RTC don't need it
>> and whether removing the alias constitutes a compatibility break.
>>
>>>> Our discussion makes me realize that the creation of the alias could now
>>>> actually be moved to the Pegasos II board. This way, the Pegasos II board
>>>> would both create and consume that alias, which seems to remove quite some
>>>> cognitive load. Do you agree? Would moving the alias to the board work for
>>>> you?
>>>
>>> Yes I think that would be better. This way the vt82xx and piix4 would be
>>> similar and the alias would also be clear within the pegasos2 code and it
>>> also has the machine directly at that point so it's clearer that way.
>>
>> All in all I wonder if we need to preserve the alias for the fuloong2e board?
>
>I don't know. A quick investigation shows that it seems to be added by commit
>654a36d857ff94 which suggests something may use it (or was intended to use it
>back then, but not sure if things have changed in the meantime). I don't think
>any management app cares about fuloong2e but if this should be a generic thing
>then all machine may need it. Then it's also mentioned in commit 29551fdcf4d99
>that suggests one ought to be careful moving this around as it may cause
>unexpected problems. But doing it from the machine init should be OK.
Then I'll create the alias in fuloong2e, too.
Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan
- Re: [PATCH 7/9] hw/isa/vt82c686: QOM'ify ac97 and mc97 creation, (continued)
[PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation, Bernhard Beschow, 2022/08/22
[PATCH 9/9] hw/isa/vt82c686: Reuse errp, Bernhard Beschow, 2022/08/22