Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

[Bernhard Beschow <shentey@gmail.com>]

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

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.

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.

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?

Thanks,
Bernhard

[1]
http://patchwork.ozlabs.org/project/qemu-devel/patch/20220205175913.31738-2-shentey@gmail.com/

>
> Regards,
> BALATON Zoltan
>

[-- Attachment #2: Type: text/html, Size: 4782 bytes --]