Re: [PATCH 2/4] hw/pci-host/prep: Remove legacy PReP machine temporary workaround

[Philippe Mathieu-Daudé]

On 10/12/20 9:28 AM, Thomas Huth wrote:
> On 12/10/2020 09.19, Philippe Mathieu-Daudé wrote:
> > The legacy PReP machine has been removed in commit b2ce76a0730
> > ("hw/ppc/prep: Remove the deprecated "prep" machine and the
> > OpenHackware BIOS"). This temporary workaround is no more
> > required, remove it.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >   hw/pci-host/prep.c | 32 +++++++++++---------------------
> >   1 file changed, 11 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> > index 80dfb67da43..064593d1e52 100644
> > --- a/hw/pci-host/prep.c
> > +++ b/hw/pci-host/prep.c
> > @@ -75,7 +75,6 @@ struct PRePPCIState {
> >       RavenPCIState pci_dev;
> >   
> >       int contiguous_map;
> > -    bool is_legacy_prep;
> >   };
> >   
> >   #define BIOS_SIZE (1 * MiB)
> > @@ -229,24 +228,18 @@ static void raven_pcihost_realizefn(DeviceState *d, Error 
> > **errp)
> >       MemoryRegion *address_space_mem = get_system_memory();
> >       int i;
> >   
> > -    if (s->is_legacy_prep) {
> > -        for (i = 0; i < PCI_NUM_PINS; i++) {
> > -            sysbus_init_irq(dev, &s->pci_irqs[i]);
> > -        }
> > -    } else {
> > -        /*
> > -         * According to PReP specification section 6.1.6 "System Interrupt
> > -         * Assignments", all PCI interrupts are routed via IRQ 15
> > -         */
> > -        s->or_irq = OR_IRQ(object_new(TYPE_OR_IRQ));
> > -        object_property_set_int(OBJECT(s->or_irq), "num-lines", PCI_NUM_PINS,
> > -                                &error_fatal);
> > -        qdev_realize(DEVICE(s->or_irq), NULL, &error_fatal);
> > -        sysbus_init_irq(dev, &s->or_irq->out_irq);
> > +    /*
> > +     * According to PReP specification section 6.1.6 "System Interrupt
> > +     * Assignments", all PCI interrupts are routed via IRQ 15.
> > +     */
>
> Since you're changing the indentation of these lines anyway, I think you
> could also simply squash patch 1 into this one here (just a matter of taste,
> though).

Then the diff is not trivial to review :/

> > +    s->or_irq = OR_IRQ(object_new(TYPE_OR_IRQ));
> > +    object_property_set_int(OBJECT(s->or_irq), "num-lines", PCI_NUM_PINS,
> > +                            &error_fatal);
> > +    qdev_realize(DEVICE(s->or_irq), NULL, &error_fatal);
> > +    sysbus_init_irq(dev, &s->or_irq->out_irq);
> >   
> > -        for (i = 0; i < PCI_NUM_PINS; i++) {
> > -            s->pci_irqs[i] = qdev_get_gpio_in(DEVICE(s->or_irq), i);
> > -        }
> > +    for (i = 0; i < PCI_NUM_PINS; i++) {
> > +        s->pci_irqs[i] = qdev_get_gpio_in(DEVICE(s->or_irq), i);
> >       }
> >   
> >       qdev_init_gpio_in(d, raven_change_gpio, 1);
> > @@ -403,9 +396,6 @@ static Property raven_pcihost_properties[] = {
> >       DEFINE_PROP_UINT32("elf-machine", PREPPCIState, pci_dev.elf_machine,
> >                          EM_NONE),
> >       DEFINE_PROP_STRING("bios-name", PREPPCIState, pci_dev.bios_name),
> > -    /* Temporary workaround until legacy prep machine is removed */
> > -    DEFINE_PROP_BOOL("is-legacy-prep", PREPPCIState, is_legacy_prep,
> > -                     false),
> >       DEFINE_PROP_END_OF_LIST()
> >   };
> >   
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!